navikt / aksel

NAVs designsystem og Aksel-portalen
https://aksel.nav.no
MIT License
158 stars 42 forks source link

[Darkside] List CSS update #3354

Closed KenAJoh closed 1 week ago

changeset-bot[bot] commented 2 weeks ago

⚠️ No Changeset found

Latest commit: 594f570092b40c3408c65e6705de14f1a7f05695

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions[bot] commented 2 weeks ago

Storybook demo / Chromatic

📝 Endringer til review: 1

288097fc0 | 91 komponenter | 135 stories

JulianNymark commented 2 weeks ago

^ consider the latest commit here a suggestion (it was just easier to commit than to do a diff in the github editor / comment system when I already had the code changes on disk 😅 )

HalvorHaugan commented 2 weeks ago

^ consider the latest commit here a suggestion (it was just easier to commit than to do a diff in the github editor / comment system when I already had the code changes on disk 😅 )

Before this change, the selectors were "grouped" logically. All selectors targeting navds-list__item was next to each other, and those having to do with the marker was next to each other. (I should have had headings though.) Do we think that the benefits of nesting outweighs the benefits of logical grouping? (Not meant as a leading question.)

JulianNymark commented 2 weeks ago

Do we think that the benefits of nesting outweighs the benefits of logical grouping? (Not meant as a leading question.)

Good question 🤔. I can only argue for it being nice to somewhat locate the same hierarchy structure in CSS as in the DOM. But then you can't really group them per leaf-node (since the same leaf-node might appear at different parts of the tree).

There will also never be "one CSS tree to rule all styling" for any component either, though that would be cool in a simplicity sense 😅. But it's just not realistic to repeat yourself several times throughout the CSS tree just to ensure it stays "within" that tree. Targeting a BEM class is just too nice when you know you want to style it across all its locations in the DOM.

I'm not sure if the grouping by BEM target / leaf-node is a perfect strategy either, sometimes you get some "what goes where?" issues regardless? I'm thinking about stuff with a A:has(.B)(does this belong grouped with A, or B)? 🤔 (side note, we don't use that many has selectors, so might be a dumb example... it's what came to mind though)

JulianNymark commented 2 weeks ago

^ To answer my own thoughts, I do think the A:has(.B) should be grouped with A, since it eventually targets that element (style gets applied to A), even though it's contingent on B. 🤔

JulianNymark commented 2 weeks ago

I think the "ultimate colocating" of styling with what should get the styling is some sort of CSS-in-js 🤔 (stylex when!?). But we also want to generate those per component plain css files as well as a global stylesheet... I would have to look more into something like https://stylexjs.com/docs/learn/ to see if its something we could use 👀, I remember it was still pretty fresh last time I heard about it.

JulianNymark commented 2 weeks ago

One more stream of consciousness rant here (thinking lead to more thinking). I'm not sure if us now grouping as much as possible by CSS nesting is completely incompatible with BEM target grouping, as long as the remaining CSS (that which doesn't fit inside a bigger tree) that is still grouped by BEM target? at least that makes sense to keep doing.

And I think that if you Ctrl+F for the BEM leaf-node you will still find all the locations without issue (you just can't search for multiple nodes anymore, do search: .my__button--special, do not search: parent > .my__button--special) 🤔.