New Admin UI - Accordion Component #4505
Conversation
…into feat/accordion
| {...rootProps} | ||
| className={cn( | ||
| "w-full", | ||
| { "wby-gap-xs wby-flex wby-flex-col": variant === "container" }, |
There was a problem hiding this comment.
Why not use the cva utility?
| "wby-border-b-sm wby-border-b-neutral-dimmed data-[state=open]:wby-rounded-bl-lg data-[state=open]:wby-rounded-br-lg", | ||
| { | ||
| "wby-rounded-lg": variant === "container", | ||
| "wby-bg-neutral-base": background === "base", | ||
| "wby-bg-neutral-light": background === "light" | ||
| } |
| ); | ||
| }; | ||
|
|
||
| AccordionItemBase.displayName = "AccordionItem"; |
There was a problem hiding this comment.
Do we need this? Maybe not, since we are not using ref
| ); | ||
| }; | ||
|
|
||
| AccordionBase.displayName = "Accordion"; |
There was a problem hiding this comment.
Do we need this? Maybe not, since we are not using ref
| animation: { | ||
| "accordion-down": "accordion-down 0.2s ease-out", | ||
| "accordion-up": "accordion-up 0.2s ease-out", | ||
| skeletonPulse: "skeletonPulse 1400ms ease-in-out infinite" |
There was a problem hiding this comment.
Shall we stick to a camelCase or kebab-case naming convention? The kebab-case looks good, but I would change skeletonPulse 🙏
|
|
||
| const AccordionContext = React.createContext<Pick<AccordionProps, "variant" | "background">>({}); | ||
|
|
||
| const useAccordion = () => { |
There was a problem hiding this comment.
The AccordionContext holds variant and background values accessible to inner components.
I had a similar situation with the Toast component, and I managed to style the inner components by adding wby-group and the variation class names to the root component -> https://github.com/webiny/webiny-js/blob/feat/new-admin-ui/packages/admin-ui/src/Toast/components/Root.tsx#L12-L15
Then, I can target the parent class name from the inner component and style it differently based on the variation class name -> https://github.com/webiny/webiny-js/blob/feat/new-admin-ui/packages/admin-ui/src/Toast/components/Title.tsx#L15
What do you think?
packages/admin-ui/src/Icon/Icon.tsx
Outdated
| @@ -29,12 +29,12 @@ const iconVariants = cva("", { | |||
| interface IconProps | |||
| extends Omit<React.HTMLAttributes<HTMLOrSVGElement>, "color">, | |||
| VariantProps<typeof iconVariants> { | |||
| label: string; | |||
| label?: string; // We don't want to force the label to be required. | |||
There was a problem hiding this comment.
I understand we don't want to force this property, but it's actually quite useful if we aim to have an accessible application. It's okay to make it optional, but providing a label should be encouraged as a best practice.
…into feat/accordion # Conflicts: # packages/cli/files/references.json
TODO
Changes
This PR creates the Accordion component.
Some screenshots.
Additional Changes
1. Icon Component -
labelProp OptionalA lot of times, I needed to use the Icon component, without the need to provide the label for the icon. So, I made the
labelprop optional.2. Removed Redundant Comments
While working on the
wby-prefix, code generation left comments in a couple of places. For example:I removed these.
3. Reordered
keyframesandanimationIntailwind.config.jsI had to add
keyframes/animationintotailwind.config.js, and then I noticed these already got here via the Skeleton PR. All good, but I still just alphabetically ordered the properties.4. Form Builder - Minor Tweaks
Had to do a bit of tweaking there. Nothing special. But it is included in this PR.