-
Notifications
You must be signed in to change notification settings - Fork 8
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
Automatic theme / Runtime Configuration HLD #1257
base: main
Are you sure you want to change the base?
Conversation
|
||
Keep the name `theme-provider`. It actually seems like a name used in many design systems, i.e. [Material UI](https://mui.com/material-ui/customization/theming/#theme-provider), [Styled Components](https://styled-components.com/docs/advanced), [Vuetify](https://vuetifyjs.com/en/components/theme-providers/), [Amazon Amplify](https://ui.docs.amplify.aws/react/theming/theme-provider) from some quick googling. | ||
|
||
I'm actually more convinced now that we shouldn't change it just because the existing terminology ("theme provider") and implementation (a component wrapping a chunk of the tree to override config) seems so common among design systems. I'll let reviewers give feedback. |
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'm all for keeping the existing name and avoiding breaking backwards compatibility.
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.
sold!
|
||
During prototyping in various prime days one thing I tried to address but found challenging was what the behavior should be if multiple global configuration elements were used on a page. I think it's unlikely to be an issue / could be something we try to address if it seems like something multiple clients are accidentally running into. The behavior will be whichever global configuration element most recently updated a configuration token will have set the value. Won't try and track them or guess which should be allowed or blocked, etc. | ||
|
||
Proposed name: `nimble-global-configuration-provider` (but should align as the `-global-` version of whatever the decided local configuration name is) |
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.
If the existing element stays as nimble-theme-provider
, do you still think this makes sense to be nimble-global-configuration-provider
, or do you think it should match and be nimble-global-theme-provider
? If they have matching APIs, then having matching names might be less confusing.
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.
Yea definitely match the local version, i.e. nimble-theme-provider
+ nimble-global-theme-provider
. Didn't see any dissenting opinions so going with those!
|
||
Note: This only addresses the top-level `<html dir>` definition and does not try to address the `dir` attribute placement on arbitrary children in the tree. Being responsive to the `<html dir>` seems like a strict improvement on current behavior and more sophisticated behavior can be investigated in the future. | ||
|
||
Breaking change: For pages that do not currently have a wrapping theme-provider they will observe that the page now responds to the system theme configuration. The docs say a wrapping theme-provider is required even though it really isn't and is hard coded to the light theme. So technically, for an app that correctly followed the docs, this is not a breaking change. |
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 didn't notice a theme provider on the nimble homepage and it does use CSS color tokens. Will this change mean that it starts respecting prefers-color-scheme
? If so we should test it to ensure we're using all the right tokens so it still looks good in dark mode.
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.
Doesn't look like any of the color theme aware tokens used in the css actually apply to elements on the page so technically not breaking :P
But it SHOULD be updated to show off theme-awareness along with Blazor and Angular apps. 👍
|
||
Have the `direction` configuration token react via MutationObserver to the `<html>` `dir` attribute configuration. See [FAST discussion](https://github.com/microsoft/fast/issues/5547#issuecomment-1027419532) about how the direction token should align with the `dir` attribute. See [W3C dir recommendations](https://www.w3.org/International/questions/qa-html-dir#quickanswer). | ||
|
||
Note: This only addresses the top-level `<html dir>` definition and does not try to address the `dir` attribute placement on arbitrary children in the tree. Being responsive to the `<html dir>` seems like a strict improvement on current behavior and more sophisticated behavior can be investigated in the future. |
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.
Note that in #1268 I'm proposing a new configuration token locale
which takes its initial value from the <html lang>
attribute. We considered having the Nimble token names match the HTML attribute names but I'm proposing explicitly not doing so because the Nimble tokens will (at least for now) behave differently, not responding to lang
or dir
attributes on elements other than html
.
Mostly commenting here for completeness. Assuming that direction is approved we could consider mentioning that rationale here, mentioning the locale
token here, or neither.
Pull Request
🤨 Rationale
HLD for proposed changes to make the components reactive to the system theme by default. Also adds an element to control the page default theme and refactors token layout.
👩💻 Implementation
See HLD.
🧪 Testing
N/A
✅ Checklist