-
Notifications
You must be signed in to change notification settings - Fork 45
chore: move default designsystemet theme setup #4366
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
Conversation
🦋 Changeset detectedLatest commit: f77a8d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 - themebuilder - www - |
84091a1 to
08c36fe
Compare
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.
Quick review to begin with, so we can discuss the placement of the root config file.
You might also need to update the version-packages packages command so it works in the release PR
.changeset/new-foxes-cough.md
Outdated
| "@digdir/designsystemet-theme": patch | ||
| --- | ||
|
|
||
| Deprecated: Use `@digdir/designsystemet-css/theme` for default theme or make your own using the [Theme builder](https://theme.designsystemet.no/) |
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.
| Deprecated: Use `@digdir/designsystemet-css/theme` for default theme or make your own using the [Theme builder](https://theme.designsystemet.no/) | |
| **DEPRECRATION: This is the last release for this package** | |
| - Use `@digdir/designsystemet-css/theme` for default theme or make your own using the [Theme builder](https://theme.designsystemet.no/) |
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.
Suggest using deprecated as its the term widely used and on npmjs. Depreciation is when something loses value over time. Will update with bold statement
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.
Updated
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.
Right, but I did not write "depriciation"? I wrote "deprecation"
You updated message looks fine though
|
I think we should also add a blogpost about this |
Barsnes
left a comment
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.
I think this looks good.
Lets get a review from @oddvernes as well, since this is quite a big change
Co-authored-by: Tobias Barsnes <tobias.barsnes@digdir.no>
Co-authored-by: Tobias Barsnes <tobias.barsnes@digdir.no>
45e1d95 to
b2f1c51
Compare
oddvernes
left a comment
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.
I approved the chromatic changes 👍
I don't see any issues whith this, well done! 👍
resolves #4295
This PR prepares for deprecation of
@digdir/designsystemet-themeand cleans up how tokens and themes are maintained.@digdir/designsystemet-themeinternally.internal/digdir(@internal/digdir)@digdir/designsystemet-themeWill do a follow up PR with moving
internal/design-tokenstointernal/digdir/design-tokensas these are tokens for Digdir themes. Need to update some scripts for this also so best to do it in a separate PR.