[Darkside] Accordion CSS-rewrite #3296
Conversation
|
Storybook demo3e1b92e56 | 91 komponenter | 144 stories |
| @@ -65,6 +65,7 @@ const AccordionItem = forwardRef<HTMLDivElement, AccordionItemProps>( | |||
| "navds-accordion__item--neutral": context?.variant === "neutral", | |||
| "navds-accordion__item--no-animation": !shouldAnimate.current, | |||
| })} | |||
| data-expanded={_open} | |||
There was a problem hiding this comment.
Why not use .navds-accordion__item--open?
There was a problem hiding this comment.
I saw this pattern used a lot in Radix, and quite like what it achieves:
- User can always see the current state in the dom
- Using classes always ends with some instances being "positive" and some "negative", so --closed or --open. Using
data-<state>removes that issue
This is mostly relevant for the cases where the component has styles applied for both states
There was a problem hiding this comment.
I super support this moving of state classes -> data-<state> attributes 🎉 . I don't like the "negative/positive" mix problem in css classes that inevitably happens 😩 .
| * @deprecated Prop will be removed and replaced by `size` in future versions. | ||
| * @default "small" | ||
| */ | ||
| headingSize?: "large" | "medium" | "small" | "xsmall"; |
There was a problem hiding this comment.
Should we delay deprecating headingSize until new design is launched? People will not be able to remove the prop until then anyways (without having to live with undesirable heading size temporarily).
Maybe we can do something clever and map headingSize to size temporarily? Edit: Probably a bad idea, just trying to think about how we can avoid any "breaking" design changes (to the extent you can call changed heading size a breaking change).
There was a problem hiding this comment.
Makes sense to delay it until release 👍
| text-align: left; | ||
| background: var(--ac-accordion-header-bg, var(--a-surface-transparent)); | ||
| cursor: pointer; | ||
| border: none; | ||
| position: relative; |
There was a problem hiding this comment.
you might want to add some sticky (and non alpha bg colors) to mirror the nav.no behaviour when you can't see the top of the accordion header anymore?
position: sticky;
top: 0;
background-color: white;
z-index: 1;
There was a problem hiding this comment.
I think it's a good feature 🙌, so I vote for adding it.
There was a problem hiding this comment.
Think this feature might need to be implemented by the teams themselves, because of the possible edgecases for where "top" is. If you dig down into how they handle it, they have a custom css-variable for "top" that changes based on decorator height
There was a problem hiding this comment.
I think the top being 0 should work out as long as parent has position relative no? it seemed to work out great in my testing 🤔 The header is always inside an accordion item wrapper, so that can be the top: 0 position?
There was a problem hiding this comment.
nvm, it works badly after all 😅
They need to be sticky towards the same containing element with offsets 😢
|
Should be ready for new review now 🤞
|
Description
Component Checklist 📝
@navikt/core/css/config/_mappings.js)@navikt/core/css/tokens.json)@navikt/aksel-stylelint/src/deprecations.ts)@navikt/core/react/src/index.tsand@navikt/core/react/package.json)@navikt/core/css/index.css)<Component>: <gitmoji?> <Text>.E.g. "Button: ✨ Add feature xyz.")