-
Notifications
You must be signed in to change notification settings - Fork 2.9k
RFC appearance migration #24181
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
Merged
Merged
RFC appearance migration #24181
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
de90f8f
i
286ec19
appearance
19001b4
up
d136f9e
txtarea
3d9fa55
s
b1aa5f3
color
803cf5d
update
dff18df
update
f1c0e54
m
4bcf5f4
Update rfcs/convergence/apperance-migration.md
petdud 5b8838b
Update rfcs/convergence/apperance-migration.md
petdud 537026f
Update rfcs/convergence/apperance-migration.md
petdud 6ddb6f4
token alias
4c074b3
example
efd09db
iframes
e8ba998
Apply suggestions from code review
miroslavstastny e3484b4
Address PR comments
miroslavstastny d744cd7
Merge remote-tracking branch 'upstream/master' into rfc-input
miroslavstastny 5040d9c
Use react context
miroslavstastny 913adc9
Merge remote-tracking branch 'upstream/master' into rfc-input
miroslavstastny File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| # RFC: Appearance migration | ||
|
|
||
| [@petr-duda](https://github.com/petr-duda) | ||
|
|
||
| ## Summary | ||
|
|
||
| Fluent V9 changes the default appearance of input components (`Dropdown`, `Input`, `TextArea`). V0 input components default background color is grey-ish, whereas in V9 the background color is white. | ||
|
|
||
| ## Background | ||
|
|
||
| Partners could change the background color to match the previous version of Fluent by passing a prop `appearance` with value `filled-darker`, but this solution does not scale well for partners when migrating to the new version and is error prone. | ||
|
|
||
| ## Problem statement | ||
|
|
||
| This RFC explores potential solutions for partners, so they could migrate input components to V9 without the extra work of adding an additional prop to input components. | ||
|
|
||
| Partners should also easily revert the decision to the default appearance value if they decide so, without changing inputs individually. | ||
|
|
||
| 👎 Cons of adding appearance to achieve the same design as V0: | ||
|
|
||
| - Not scalable, partners would have to add the prop to every component | ||
| - If they decide to revert back to default appearance in future, they would have to go through the same pain again by removing the prop | ||
| - Is error prone | ||
|
|
||
| ## Detailed Design or Proposal | ||
|
|
||
| ### Compose components on application side | ||
|
|
||
| Partners could create a new composed component on application side. and modify the props in their preferred way. If the partner would like to keep the original color, they could create the composed component and have the default component without the appearance prop renders as `filled-darker`. | ||
|
|
||
| #### Example | ||
|
|
||
| ```ts | ||
| export const Input: ForwardRefComponent<InputProps> = React.forwardRef((props, ref) => { | ||
| const state = useInput_unstable({ appearance: 'filled-darker', ...props }, ref); | ||
|
|
||
| useInputStyles_unstable(state); | ||
| return renderInput_unstable(state); | ||
| }); | ||
| ``` | ||
|
|
||
miroslavstastny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 👍 Pros: | ||
|
|
||
| - Is relatively safe | ||
|
|
||
| 👎 Cons: | ||
|
|
||
| - Creating new composed component for each input `Dropdown`, `Input`, `TextArea` and `DatePicker` | ||
| - Composed component apperance prop wouldn't match our Fluent V9 documentation | ||
| - Does not work if repos have dependencies of another project with Fluent V9 input components (Nova) | ||
| - Composition approach is currently marked as unstable in v9 | ||
|
|
||
| ### Wrap the library components on application side | ||
|
|
||
| Partners could a new component wrapper that will wrap input components and modify the appearance prop on the application side. | ||
|
|
||
miroslavstastny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #### Example | ||
|
|
||
| ```tsx | ||
| export const Input: ForwardRefComponent<InputProps> = React.forwardRef((props, ref) => { | ||
| return <BaseInput appearance="filled-darker" {...props} ref={ref} />; | ||
| }); | ||
| ``` | ||
|
|
||
| 👍 Pros: | ||
|
|
||
| - Is safe and stable | ||
|
|
||
| 👎 Cons: | ||
|
|
||
| - Creating new composed component for each input `Dropdown`, `Input`, `TextArea` and `DatePicker` | ||
miroslavstastny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - Wrapper components in React's i.e. bigger React tree | ||
| - Wrapper component apperance wouldn't match our Fluent V9 documentation | ||
| - Would work in iframes only within the TMP repository | ||
| - Does not work if repos have dependencies of another project with Fluent V9 input components (Nova) | ||
|
|
||
petdud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ### Global css selector | ||
|
|
||
| Targeting all input selectors from a partner app and change the color with global css. | ||
petdud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| Example: https://codesandbox.io/s/rfc-inputs-ew821q?file=/app.tsx | ||
petdud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 👍 Pros: | ||
|
|
||
| - Relatively easy and fast to do | ||
| - Scalable | ||
| - Would work for dependencies such as Nova (but depending where we inject the CSS rule and if they have their own `FluentProvider`) | ||
|
|
||
| 👎 Cons: | ||
|
|
||
| - Difficult to validate | ||
|
|
||
| ### New token alias to theme | ||
petdud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| Adding a new alias color token (let's say `colorInputBackground`) and use it for all inputs. | ||
|
|
||
| 👍 Pros: | ||
|
|
||
| - Easy to do and would work for all inputs | ||
|
|
||
| 👎 Cons: | ||
|
|
||
| - Negative impact on performance by increasing variables (as read here: [fluentui/theme-shared-colors.md at d5d510bf1ffcc1a4ed2067e9eb009c84e7beb351 · microsoft/fluentui (github.com)](https://github.com/microsoft/fluentui/blob/d5d510bf1ffcc1a4ed2067e9eb009c84e7beb351/rfcs/react-components/convergence/theme-shared-colors.md)) | ||
miroslavstastny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - Divergence themes from the original | ||
|
|
||
| ### New optional token with fallback to a theme token | ||
|
|
||
| Another option is to add a possibility to override the background using a CSS variable: | ||
| We can use `backgroundColor: var(--inputBackgroundOverride, ${tokens.colorNeutralBackground1})` without setting the `--inputBackgroundOverride` anywhere. Then an application can set that variable if it needs to override the background. | ||
|
|
||
| 👍 no additional tokens unless needed | ||
|
|
||
| 👎 new concept | ||
| 👎 one-off just for the Input background (although it would be used in multiple input components) | ||
|
|
||
| ### Unify design | ||
|
|
||
| Discuss with designers to unify V0 and V9 design, setting the appearance to filled-dark by default. | ||
|
|
||
| 👍 Props: | ||
|
|
||
| - Will make migration easier for partners who already uses V0 | ||
| - Would work with iframes (if all the teams have the unified design) | ||
|
|
||
| 👎 Cons: | ||
|
|
||
| - Inherits design from old V0 package that does not meet our needs/goals | ||
petdud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - According to the design team, this is currently a no-go | ||
miroslavstastny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### Use React context | ||
|
|
||
| Another option is to use React Context to override `appearance` defaults in `FluentProvider`. | ||
|
|
||
| #### Example | ||
|
|
||
| ```tsx | ||
| function App() { | ||
| return ( | ||
| <FluentProvider appearance="filled-darker"> | ||
| <Input /> {/* has "filled-darker" */} | ||
| <Input appearance="underline" /> {/* has "underline" */} | ||
| </FluentProvider> | ||
| ) | ||
| } | ||
|
|
||
| 👍 more universal solution than a custom token - can be used to override different concepts, not only tokens - props, icons, etc. | ||
|
|
||
| 👎 new concept | ||
| 👎 one-off just for the Input background (although it would be used in multiple input components) | ||
| ``` | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.