Fix Accordion Component #4754
Fix Accordion Component #4754
Conversation
- Add `background` prop with options: "base", "light", "transparent" - Set default background to "light" for consistency - Update nested accordion background logic to respect explicit prop - Remove hardcoded margin line from AccordionTrigger - Update documentation and Storybook examples with new prop - Add background control to Storybook Documentation story
- Wrap Icon component in a div to allow className prop - Add wby-pl-md padding class to OpenCloseIndicator - Improves visual spacing in accordion trigger layout
Hide the error icon that was previously displayed in the content entries validation indicators by setting display to none instead of applying error icon styles.
| const AccordionExample = () => { | ||
| return ( | ||
| <Accordion variant="underline"> | ||
| <Accordion variant="underline" background="base"> |
There was a problem hiding this comment.
Can we rather invert the logic here?
Instead of having the default bg being as 'alternating' (or whatever the most appropriate name would be here), can we have base (white) as the default, and then have the user specify 'alternating' explicitly, if required for the use case they are buildig?
Because it's like..... in most cases, you don't need this alternating bg. So far we only have it for obj fields. The rest of usages throughout the Admin app are basic, with base bg.
Wdyt?
There was a problem hiding this comment.
can we have base (white) as the default, and then have the user specify 'alternating' explicitly, if required for the use case they are buildig?
But the base background is already default. What I did here is that I only reverted to how it was before.
There was a problem hiding this comment.
Because it's like..... in most cases, you don't need this alternating bg. So far we only have it for obj fields. The rest of usages throughout the Admin app are basic, with base bg.
Wdyt?
Alternating background works independently from background prop. If background prop is given, alternating backgrounds are skipped.
- Change default accordion background from "light" to "base" - Inline splitAccordionProps function into AccordionItemBase - Restructure props destructuring for better readability - Maintain same functionality with cleaner implementation The props splitting logic is now contained within a useMemo hook directly in the component, removing the need for a separate utility function and making the code flow more straightforward.
- Move open/close indicator back to right side after actions - Remove `isInteractable` variable in favor of inline conditions - Add separator between actions and indicator when both present - Change icon conditional from `&&` to ternary for consistency This restores the original layout behavior while maintaining cleaner conditional logic.
ku5ic
force-pushed
the
ku5ic/fix/accordion_component
branch
from
1c74c05 to
15070fd
Compare
| const getBackgroundByDepth = ( | ||
| depth: number, | ||
| background?: string | null | ||
| ): "base" | "light" | "transparent" | undefined => { | ||
| // If background is explicitly provided, use it at any depth | ||
| if (background !== undefined && background !== null) { | ||
| return background as "base" | "light" | "transparent"; | ||
| } | ||
|
|
||
| // For depth 0, return undefined to let defaultVariants apply | ||
| if (depth === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // For nested levels, alternate between light and base | ||
| return depth % 2 === 0 ? "light" : "base"; |
There was a problem hiding this comment.
I started with suggesting the below refactor. But then, I noticed another thing. So, let's ignore the refactor for now.
My question here is....
Seems to me that, if depth === 0, then, the returning bg value will be base.
Then, for the next depth, which is 1, the resulting modulo result will be 1, in which case the return depth % 2 === 0 ? "light" : "base"; statement again returns base as the bg value.
Which means, we would have base bg for to depths in a row.
Maybe I got something wrong, but, is this correct?
Tnx!
--
Note
We can discuss this nitpick refactor later. Let's focus on the above first.
What would you say about this refactor?
const getBackgroundByDepth = (
depth: number,
background: AccordionProps["background"]
): AccordionProps["background"] => {
// If background is explicitly provided, use it at any depth
if (background) {
return background
}
// For nested levels, alternate between light and base
return depth % 2 === 0 ? "light" : "base";
};Basically, instead of copying background values, we reuse the AccordionProps["background"] type. Easier to maintain.
Second, instead of partially relying on the default bg set via cva above (the // For depth 0, return undefined to let defaultVariants apply if statement), we contain the value to be fully determined by the function itself. This way, the logic is more localised.
It's a nitpick, not blocking anything. Just providing an alternative way to impl it.
There was a problem hiding this comment.
Addressed with fe2d5b6
| >; | ||
|
|
||
| const OpenCloseIndicator = () => { | ||
| const OpenCloseIndicator = ({ className }: { className?: string }) => { |
There was a problem hiding this comment.
Do we need the div wrapper here? I think not, but just dbl checking.
There was a problem hiding this comment.
Addressed with 509dede
- Remove unnecessary null checks and type assertions - Simplify getBackgroundByDepth return type to use AccordionProps type - Remove redundant depth === 0 check for undefined handling - Improve code readability while maintaining same functionality
Remove unnecessary wrapper div from OpenCloseIndicator component and its className prop, simplifying the component structure.
Even depths (0, 2, 4...): Returns
|
3 participants
Changes
This pull request introduces enhancements to the
Accordioncomponent in the admin UI, primarily by adding support for configurable background colors and improving its documentation and storybook integration. The changes make it easier to set and preview different background styles for both top-level and nested accordions, and also include a minor refactor to the open/close indicator for better styling flexibility.Accordion background configuration and styling
backgroundprop to theAccordioncomponent, allowing selection among"base","light", or"transparent"backgrounds. Updated the default variant and background inAccordion.tsxand improved logic for nested accordions to alternate backgrounds or use the explicitly provided value.Accordion.mdxand storybook stories inAccordion.stories.tsxto demonstrate and document the newbackgroundprop, including adding controls for background selection in storybook.Component refactor and visual improvements
OpenCloseIndicatorcomponent to accept aclassNameprop for improved styling flexibility, and updated its usage to add padding when interactable.How Has This Been Tested?
n/a
Documentation
n/a