Automatic theme / Runtime Configuration HLD #1257
Conversation
rajsite
requested review from
jattasNI,
atmgrifter00 and
mollykreis
as code owners
rajsite
changed the title
|
|
||
| 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.
I'm all for keeping the existing name and avoiding breaking backwards compatibility.
|
|
||
| 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.
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.
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.
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.
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.
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