-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: css modules #2318
feat: css modules #2318
Conversation
🦋 Changeset detectedLatest commit: 014641a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Preview deployments for this pull request: 📖 Storybook 🖥 Storefront See all deployments at https://dev.designsystemet.no |
I am not sure if having one package makes it easier for the consumer.
Having one package makes it easier to update, but if we keep all packages in sync, this is not a problem. |
packages/css/baseline/ds-reset.css
Outdated
/* TODO: Can this be moved to the related components? */ | ||
[class^='ds-'], | ||
[class^='ds-']::before, | ||
[class^='ds-']::after { | ||
box-sizing: border-box; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is relevant for all components, it's the same as doing
* { box-sizing: border-box }
So I think this is fine to have in this file 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not if moving to CSS modules or allowing people to extend our classes like:
.my-fancy-button {
composes: button from '@digdir/designsystemet/button.module.css'; // To extend using pure CSS
…
}
Also, this targets only if elements class names starts with ds-
so class="ds-button my-button"
works, but class="my-button ds-button"
does not. I think it is safer and more explicit to just add box-sizing: border-box
into the relevant classes in the relevant files :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, but this should be [class~="ds-"]
😄
[class^='ds-'], | ||
[class^='ds-']::before, | ||
[class^='ds-']::after { | ||
box-sizing: border-box; | ||
} | ||
|
||
/* Inherit fonts for inputs and buttons */ | ||
|
||
/* TODO: Can this be moved to the related form element CSS files? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this is a better candididate for moving to to their relevant components.
However, it gives us another point of failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above regarding targeting start of classnames :)
And I suggest forgetting to add styling should be done in reviews, not by an "invisible" dependency relation :)
@@ -1,3 +1,4 @@ | |||
/* TODO: Could these styles be placed in css root like the other files? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
We would also need to add a script for this, to fix hashes, prefixes and support extracting pure classes without a hash |
Unsure what you mean here - Vite does this? :) and the version without hashes is just making a file
... since |
I'll elaborate on my thoughs: I am not sure how vite handles hashing, but at the same time we don't know if our consumers use vite. Edit: If we want to use features like |
To summaries our meeting on 26.08.24 with @Barsnes @Thuneer @eirikbacker: We decided to introduce css-modules for the CSS package so that our users can choose to use it if needed, be it for versioning different version or composing with their own CSS. This also has the benefit internally of giving us typesafety for our classes atleast and composing typography to reduce number classNames to get desired result, while keeping themable typography.
|
One thing, won't this remove the ability for our React components to be headless (since we have an import statement for css files inside components). |
This would indeed be correct 🤔 |
It also makes it hard for users that target our css classes without a buildstep for css modules, since it would change for every version |
@mimarz regarding headless; as @Barsnes points out; headless it not possible today either. A solution if we want to provide headless in the future is adding a @Barsnes Targeting our classnames would always be a bad idea, as we can change the classnames regardless. We should encourage people using the React package to always add their own className to extend styling |
Coverage Report
File Coverage
|
Preview deploymentsTheme |
This will be done again in #2385 |
@digdir/designsystemet-react
and@digdir/designsystemet-css
, (...and@digdir/designsystemet-js
and@digdir/designsystemet-svelte
etc.), but rather just@digdir/designsystemet
(or@digdir/designsystemet-components
) as CSS and React/JS/* must be kept in sync anyway?This makes it easier for us to maintain, and easier for the consumer, as they get one package but can use different flavours:
or:
or:
and:
and: