-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(wc): Add v9-compatible themes #25660
feat(wc): Add v9-compatible themes #25660
Conversation
@@ -14,36 +20,11 @@ export const parameters = { | |||
}, | |||
options: { | |||
storySort: { | |||
order: [ |
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.
Removed legacy
packages/web-components/package.json
Outdated
"@microsoft/fast-element": "^2.0.0-beta.17", | ||
"@microsoft/fast-foundation": "^3.0.0-alpha.21", | ||
"@microsoft/fast-web-utilities": "^6.0.0", | ||
"@fluentui/react-theme": "^9.1.1", |
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 adds dependency on a react-theme
package 💣
There is nothing React related in that package besides two things:
- It has react in the package name.
- It adds react as a peer dependency.
If we decide to go this way, we should do one of the following things:
- Rename
react-theme
totheme
, get rid of the peer dependency (might be breaking change for FUIRv9). - Hoist the tokens from
react-theme
totheme
, re-export inreact-theme
. - Use token pipeline to generate the same theme files for
web-components
.
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.
ideally we should use same fluent tokens and themes in all web experiences, thus
if react-theme
doesn't contain anything react specific besides the peerDep
- deprecate
react-theme
on npm - rename
react-theme
toreact
(removal of peer Deps )- having react-theme with re-exports of theme adds unnecessary maintenance
might be breaking change for FUIRv9
from react-components api POV there will be no breaking change. once someone installs new feature bump of react-components they will get theme
instead of react-theme
in they node_modules
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 there is no use of React in react-theme
why can't we just create a theme
package and use that directly in react-components
and get rid of the react-theme
all together?
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.
That's what @Hotell is proposing (+ npm deprecation). I was not sure whether we would consider it a breaking change or not.
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.
Agree that the change is not breaking.
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.
Was just chatting with @chrisdholt today and we'd definitely love to have a shared, library-agnostic theme package that we can base both React and Web Components on. Sounds like folks here arrived at the same conclusion. +1 from me.
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.
+1 lets start from here
|
||
const { create } = DesignToken; | ||
|
||
export const borderRadiusNone = create<string>('borderRadiusNone'); |
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.
No default values here. Each theme has its own values.
@@ -0,0 +1,383 @@ | |||
import { DesignToken } from '@microsoft/fast-foundation'; |
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 files should be generated by the token pipeline.
@@ -0,0 +1,2 @@ | |||
export * from './design-tokens'; |
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 need to add bundle-size / tree-shaking tests. After that we might reconsider the way how we export and consume tokens.
* Sets the theme tokens on defaultNode. | ||
* @param theme Flat object of theme token values. | ||
*/ | ||
export const setTheme = (theme: Theme) => { |
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.
Only defaultNode
(global) for now.
If agreed, we will add setValueFor
version for scoped theming.
import { setTheme } from '../src/theme'; | ||
|
||
const themes = { | ||
'web-light': webLightTheme, |
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.
For now, there are no theme exports from web components.
If we decide to go this way, we need to discuss the export contract.
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.
Is this for Storybook only? Named exports are treeshakable but an object is not, so all themes will always be included here.
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.
Correct, storybook only for now.
Once we know the packages structure, we will decide whether partners will import the theme directly from theme
package or web-components
will re-export the themes and how.
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.
We can discuss later - may be ideal to export from the package though given we’re using DesignToken.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 8c9a0f3:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 7f15428e8fb2c3cfbfe8e555978bfa66f74f8fd8 (build) |
📊 Bundle size reportUnchanged fixtures
|
you have dep dupes. make sure to run |
@@ -0,0 +1,9 @@ | |||
<div id="switches-container"> |
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.
good opportunity to leverage here our theme picker (storybook addon) , later
packages/web-components/package.json
Outdated
"@microsoft/fast-element": "^2.0.0-beta.17", | ||
"@microsoft/fast-foundation": "^3.0.0-alpha.21", | ||
"@microsoft/fast-web-utilities": "^6.0.0", | ||
"@fluentui/react-theme": "^9.1.1", |
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.
ideally we should use same fluent tokens and themes in all web experiences, thus
if react-theme
doesn't contain anything react specific besides the peerDep
- deprecate
react-theme
on npm - rename
react-theme
toreact
(removal of peer Deps )- having react-theme with re-exports of theme adds unnecessary maintenance
might be breaking change for FUIRv9
from react-components api POV there will be no breaking change. once someone installs new feature bump of react-components they will get theme
instead of react-theme
in they node_modules
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
<h3>Theme Tokens</h3> | ||
<p>Debug story which uses theme tokens to style the element below.</p> | ||
<div style=" | ||
font-family: ${tokens.fontFamilyBase.createCSS()}; |
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.
@nicholasrice What do you think about adding a toString()
and valueOf()
implementation to CSSDesignToken
? That would make this syntax nicer if used outside of a FAST html
or css
template.
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.
toString()
seems fine to me. Can you elaborate more on valueOf()
? It's not clear to me what that would return.
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.
Actually, I don't think we need valueOf()
. I just checked and the JavaScript runtime prefers toString()
for string conversions over valueOf()
. Since we don't need to convert to a numerical type of some non-string type we should be fine with toString()
.
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.
Oh I see, I didn't realize that was an object prototype method. I think adding toString()
to CSSDesignToken
makes sense.
As agreed offline, tokens were hoisted from |
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.
Approving for v-build related changes
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.
Let’s move this through and we can adjust iterate as needed (such as exports, etc). Thanks @miroslavstastny!
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
* bump FAST dependencies * Add theme tokens (from react-theme) * Add theme switcher to storybook * deduplicate packages * fix yarn.lock * Replace @fluentui/react-theme with @fluentui/tokens * change file * address PR comments
Add themes compatible with
@fluentui/react-components@9
In v9, there are currently 4 basic themes:
Each of the themes is a function of a brand ramp. As an example Web Light Theme is the Light Theme created with Web brand ramp and Teams Light Theme is the Light Theme created with Teams brand ramp.
This PR proposed how Web Components can have a theme compatible with React.