From a44d78ffcff156bb0108766af4d98cb833895637 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Fri, 27 May 2022 17:45:56 +0200 Subject: [PATCH 01/11] feat: add new RFC --- .../no-action-prop-value-standardization.md | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 rfcs/react-components/no-action-prop-value-standardization.md diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md new file mode 100644 index 00000000000000..6d156b64c882ce --- /dev/null +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -0,0 +1,80 @@ +# RFC: "No action" / default property value standardization + +--- + +_@andrefcdias @Hotell_ + +## Summary + +This RFC aims to standardize what values we use in our components for cases where a prop value results in no action, i.e. defaults that apply no styles. + +## Background + +Currently, the approach we follow for all components is to use a string value like 'off', 'none' or 'default' for default values of a prop. + +## Problem statement + +There is no standardization for the naming, resulting in our users needing to read documentation to figure out what to use and requiring specific component knowledge as they can't reuse this information for other components. Given that we use TypeScript's `Required` to enforce these attributes to be non-nullable, a user will have to read through all the docs of said component to figure out what to use, when consuming a `useComponentStyles_unstable` hook. For a user that wants to consume the `useTextStyles` to apply only bold, they would need to write: + +```ts +useTextStyles_unstable({ + align: 'start', + block: false, + font: 'base', + italic: false, + size: 300, + strikethrough: false, + truncate: false, + underline: false, + weight: 'semibold', // Only change from defaults + wrap: true, +}); +``` + +## Detailed Design or Proposal + +This RFC proposes that we leverage the standard JavaScript default, `undefined`, for attributes instead of a string. +For this, we should stop using `Required` for all props in the `State` and decide case by case if something should / should not be optional. + +Cases where our component should have a default value, we would apply the state that should be the default value. +i.e. `Card` component's `appearance` prop, should have a default appearance, as we don't want a card without a pre-defined appearance as that's detrimental to the usage. + +Cases where nothing is done in our hooks as a default, we would leverage `undefined` and `null`. +i.e. `Card` component's `focusMode` prop should be nullable instead of using an `off` string value, as that reflects the fact that nothing is applied to the component. + +### Pros and Cons + +#### Pros + +- Follows JavaScript standards +- Intuitive to the user because of the above pro - no need to read documentation +- Consistency across our product - enables ease of use + +#### Cons + +- New style map approach would require changes + ```ts + const alignMap = { + none: undefined, + start: styles.alignStart, + center: styles.alignCenter, + end: styles.alignEnd, + justify: styles.alignJustify, + } as const; + ``` +- Users who dynamically set a property value would need to do something like: + ```tsx + + ``` + +## Discarded Solutions + +### Standardizing the string label used for default values + +Using a specific keyword for all possible scenarios would be difficult as there isn't a word that is neutral and appropriate for all the different cases. + +- `none` can be confused with CSS's `none` +- `off` does not fit cases like an `align` property +- `default` is ambiguous and does not convey the fact that nothing happens + +Even if we can get a word that fits all the cases, the user would still need to add every single property when consuming a styles hook, like the example shown in the [Problem Statement](##Problem_statement). From 1ca1378c2008b71e2e63a31a434f78185f6bc13f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Mon, 30 May 2022 15:36:28 +0200 Subject: [PATCH 02/11] chore: remove `null` values from proposal --- rfcs/react-components/no-action-prop-value-standardization.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index 6d156b64c882ce..1fde9143c6c3a5 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -39,7 +39,7 @@ For this, we should stop using `Required` for all props in the `State` and decid Cases where our component should have a default value, we would apply the state that should be the default value. i.e. `Card` component's `appearance` prop, should have a default appearance, as we don't want a card without a pre-defined appearance as that's detrimental to the usage. -Cases where nothing is done in our hooks as a default, we would leverage `undefined` and `null`. +Cases where nothing is done in our hooks as a default, we would leverage `undefined`. i.e. `Card` component's `focusMode` prop should be nullable instead of using an `off` string value, as that reflects the fact that nothing is applied to the component. ### Pros and Cons From c46c4a2eb9974b20e9cca37e0525cf706e3320b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Mon, 30 May 2022 15:52:39 +0200 Subject: [PATCH 03/11] chore: add proposal to fix lookup pattern con --- .../no-action-prop-value-standardization.md | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index 1fde9143c6c3a5..5c56c8399e0db6 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -42,6 +42,10 @@ i.e. `Card` component's `appearance` prop, should have a default appearance, as Cases where nothing is done in our hooks as a default, we would leverage `undefined`. i.e. `Card` component's `focusMode` prop should be nullable instead of using an `off` string value, as that reflects the fact that nothing is applied to the component. +```tsx + +// Text.types.ts +type TextProps = { + font?: 'base' | 'monospace' | 'numeric'; +}; +type TextState = { + // Also nullable as the default does not overwrite styles + font?: 'base' | 'monospace' | 'numeric'; +}; + +// useText.ts +const { font /* {...} */ } = props; // We no longer set the default here + +const state = { + font, + // {...} +}; + +// useTextStyles.ts +const { font = '_default' } = state; // Applying a default to keep using a lookup + +const fontLookup = { + _default: undefined, + base: styles.fontBase, + monospace: styles.fontMonospace, + numeric: styles.fontNumeric, +} as const; + +state.root.className = mergeClasses( + cardClassNames.root, + styles.root, + fontLookup[font], + // {...} + state.root.className, +); +``` + +### Usage difference + +```ts +// Before +useTextStyles_unstable({ + // User has to check docs to double check what the default shoud be. + align: 'start', + block: false, + font: 'monospace', // Only change from defaults + italic: false, + size: 300, + strikethrough: false, + truncate: false, + underline: false, + weight: 'regular', + wrap: true, +}); + +// After +useTextStyles_unstable({ + font: 'monospace', + // {...} - Required props +}); ``` -### Pros and Cons +## Pros and Cons -#### Pros +### Pros - Follows JavaScript standards - Intuitive to the user because of the above pro - no need to read documentation - Consistency across our product - enables ease of use -#### Cons +### Cons - Using the lookup object pattern for styling would require changes From 81fde29b23dc9d445c7aeffd726422817a1db57d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Mon, 6 Jun 2022 06:58:17 +0200 Subject: [PATCH 05/11] chore: add unifying of shape pro --- rfcs/react-components/no-action-prop-value-standardization.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index 017c5bd68c6ee6..dec3e7dd017ade 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -146,6 +146,7 @@ useTextStyles_unstable({ - Follows JavaScript standards - Intuitive to the user because of the above pro - no need to read documentation - Consistency across our product - enables ease of use +- Unified `Props` and `State` shape ### Cons From 226b1e699d23694d62b0c1229f3540c3a93f5a55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Mon, 6 Jun 2022 12:17:20 +0200 Subject: [PATCH 06/11] chore: a nice rephrasing --- rfcs/react-components/no-action-prop-value-standardization.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index dec3e7dd017ade..72cd58403ec82a 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -74,8 +74,8 @@ state.root.className = mergeClasses( ); ``` -Cases where nothing is done in our hooks as a default, we should leverage `undefined`. -i.e. `Text` component's `font` prop should be nullable instead of using a `'default'` string value, as that reflects the fact that there are no side-effects from it. +Cases where nothing is done in our hooks as a default, we should leverage the JavaScript standard for no value - `undefined`. +i.e. `Text` component's `font` prop should be optional/undefined, instead of using a `'default'` string value, as these values reflect the fact that there are no side-effects from it. ```tsx // Text.types.ts From 3dc18783a18e3175e4886784b83634eebdbb3d72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Dias=20=28he/him=29?= <39736248+andrefcdias@users.noreply.github.com> Date: Thu, 9 Jun 2022 14:53:57 +0200 Subject: [PATCH 07/11] Update rfcs/react-components/no-action-prop-value-standardization.md Co-authored-by: Makoto Morimoto --- rfcs/react-components/no-action-prop-value-standardization.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index 72cd58403ec82a..8880bb70a07602 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -119,7 +119,7 @@ state.root.className = mergeClasses( ```ts // Before useTextStyles_unstable({ - // User has to check docs to double check what the default shoud be. + // User has to check docs to double check what the default should be. align: 'start', block: false, font: 'monospace', // Only change from defaults From 7adbf1f9b4252fd59eaf7047604dc6571e069bbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Thu, 9 Jun 2022 15:34:11 +0200 Subject: [PATCH 08/11] chore: fix usage --- rfcs/react-components/no-action-prop-value-standardization.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index 8880bb70a07602..be71aee10316ba 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -96,7 +96,7 @@ const state = { }; // useTextStyles.ts -const { font = '_default' } = state; // Applying a default to keep using a lookup +const { font } = state; const fontLookup = { _default: undefined, @@ -108,7 +108,7 @@ const fontLookup = { state.root.className = mergeClasses( cardClassNames.root, styles.root, - fontLookup[font], + fontLookup[font ?? '_default'], // Applying a default to keep usage of a lookup // {...} state.root.className, ); From 987a00a98c43f78cabc05521f313b3cdaaf3da34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Fri, 10 Jun 2022 17:55:16 +0200 Subject: [PATCH 09/11] chore: simplify proposal --- .../no-action-prop-value-standardization.md | 48 +++++++++++++------ 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index be71aee10316ba..3de80bc35038dc 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -36,8 +36,16 @@ useTextStyles_unstable({ This RFC proposes that we leverage the standard JavaScript default, `undefined`, for attributes instead of a string. For this, we should stop using `Required` for all props in the `State` and decide case by case if something should / should not be optional. -Cases where our component should have a default value, we would apply a primitive value for it, like a string or a boolean. -i.e. `Card` component's `appearance` prop, should have a default value, as we don't want a card without a pre-defined appearance, as that's detrimental to the usage. +Put simply, when a component has a default state/behavior, it SHOULD HAVE a default value.
+Example: Card.appearance has `'filled' | 'filled-alternative' | 'outline' | 'subtle'` and is `'filled'` by default. + +When a component does not have a default state/behavior, it SHOULD NOT HAVE a default value.
+Anti-example (current state): Text.font has `'base' | 'monospace' | 'numeric'` and defaults to `'base'`, where `'base'` does not apply any styles
+Example (proposal): Text.font has `'monospace' | 'numeric'` and has no defaults (i.e. `undefined`) + +### Usage + +#### With default ```tsx // Card.types.ts @@ -74,21 +82,20 @@ state.root.className = mergeClasses( ); ``` -Cases where nothing is done in our hooks as a default, we should leverage the JavaScript standard for no value - `undefined`. -i.e. `Text` component's `font` prop should be optional/undefined, instead of using a `'default'` string value, as these values reflect the fact that there are no side-effects from it. +#### Without default ```tsx // Text.types.ts type TextProps = { - font?: 'base' | 'monospace' | 'numeric'; + font?: 'monospace' | 'numeric'; }; type TextState = { // Also nullable as the default does not overwrite styles - font?: 'base' | 'monospace' | 'numeric'; + font?: 'monospace' | 'numeric'; }; // useText.ts -const { font /* {...} */ } = props; // We no longer set the default here +const { font /* {...} */ } = props; // We no longer set a default here const state = { font, @@ -114,9 +121,21 @@ state.root.className = mergeClasses( ); ``` -### Usage difference +### Usage differences -```ts +#### Dynamically setting a prop value + +```jsx +// Before + + +// After + +``` + +#### Consuming style hooks directly + +```js // Before useTextStyles_unstable({ // User has to check docs to double check what the default should be. @@ -150,12 +169,11 @@ useTextStyles_unstable({ ### Cons -- Using the lookup object pattern for styling would require changes +- Using the lookup object pattern for styling will require changes as `undefined` can't be used as an index type Proposal: - - ```tsx + ```js const state = { align: undefined } as const; const alignMap = { @@ -173,9 +191,9 @@ useTextStyles_unstable({ ); ``` -- Users who dynamically set a property value would need to do something like: - ```tsx - +- Dynamically setting a property value requires explicit usage of `undefined`: + ```jsx + ``` ## Discarded Solutions From e37d801c0516aeb052b65a7b1a7142db967e0a54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Thu, 16 Jun 2022 21:40:07 +0200 Subject: [PATCH 10/11] chore: remove mentions of hook changes --- .../no-action-prop-value-standardization.md | 45 +------------------ 1 file changed, 2 insertions(+), 43 deletions(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index 3de80bc35038dc..a8bd0a608a6cdb 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -10,31 +10,15 @@ This RFC aims to standardize what values we use in our components for cases wher ## Background -Currently, the approach we follow for all components is to use a string value like 'off', 'none' or 'default' for default values of a prop. +Currently, the approach we follow for all components is to use a string value like `'off'`, `'none'` or `'default'` for default values of a prop. This happens both on cases where the default prop has and doesn't have an effect on the component. ## Problem statement -There is no standardization for the naming, resulting in our users needing to read documentation to figure out what to use and requiring specific component knowledge as they can't reuse this information for other components. Given that we use TypeScript's `Required` to enforce these attributes to be non-nullable, a user will have to read through all the docs of said component to figure out what to use, when consuming a `useComponentStyles_unstable` hook. For a user that wants to consume the `useTextStyles` to apply only bold, they would need to write: - -```ts -useTextStyles_unstable({ - align: 'start', - block: false, - font: 'base', - italic: false, - size: 300, - strikethrough: false, - truncate: false, - underline: false, - weight: 'semibold', // Only change from defaults - wrap: true, -}); -``` +There is no standardization for the naming, resulting in our users needing to read documentation to figure out what to use, as the names used might be compared to CSS keywords (like `'unset'`), and requiring specific component knowledge as the user can't reuse this information for other components. It is also misleading to provide a string value that has no actual impact on the component. ## Detailed Design or Proposal This RFC proposes that we leverage the standard JavaScript default, `undefined`, for attributes instead of a string. -For this, we should stop using `Required` for all props in the `State` and decide case by case if something should / should not be optional. Put simply, when a component has a default state/behavior, it SHOULD HAVE a default value.
Example: Card.appearance has `'filled' | 'filled-alternative' | 'outline' | 'subtle'` and is `'filled'` by default. @@ -133,31 +117,6 @@ state.root.className = mergeClasses( ``` -#### Consuming style hooks directly - -```js -// Before -useTextStyles_unstable({ - // User has to check docs to double check what the default should be. - align: 'start', - block: false, - font: 'monospace', // Only change from defaults - italic: false, - size: 300, - strikethrough: false, - truncate: false, - underline: false, - weight: 'regular', - wrap: true, -}); - -// After -useTextStyles_unstable({ - font: 'monospace', - // {...} - Required props -}); -``` - ## Pros and Cons ### Pros From 702650b8c0f398ff9a7e2d748f2d2bbb71ed4a6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <39736248+andrefcdias@users.noreply.github.com> Date: Tue, 12 Jul 2022 19:18:57 +0200 Subject: [PATCH 11/11] chore: remove implementation proposal --- .../no-action-prop-value-standardization.md | 39 ------------------- 1 file changed, 39 deletions(-) diff --git a/rfcs/react-components/no-action-prop-value-standardization.md b/rfcs/react-components/no-action-prop-value-standardization.md index a8bd0a608a6cdb..18aba3caaeae3f 100644 --- a/rfcs/react-components/no-action-prop-value-standardization.md +++ b/rfcs/react-components/no-action-prop-value-standardization.md @@ -85,24 +85,6 @@ const state = { font, // {...} }; - -// useTextStyles.ts -const { font } = state; - -const fontLookup = { - _default: undefined, - base: styles.fontBase, - monospace: styles.fontMonospace, - numeric: styles.fontNumeric, -} as const; - -state.root.className = mergeClasses( - cardClassNames.root, - styles.root, - fontLookup[font ?? '_default'], // Applying a default to keep usage of a lookup - // {...} - state.root.className, -); ``` ### Usage differences @@ -129,27 +111,6 @@ state.root.className = mergeClasses( ### Cons - Using the lookup object pattern for styling will require changes as `undefined` can't be used as an index type - - Proposal: - - ```js - const state = { align: undefined } as const; - - const alignMap = { - _default: undefined, - start: styles.alignStart, - center: styles.alignCenter, - end: styles.alignEnd, - justify: styles.alignJustify, - } as const; - - const className = mergeClasses( - classNames.root, - styles.root, - alignMap[props.align ?? '_default'] - ); - ``` - - Dynamically setting a property value requires explicit usage of `undefined`: ```jsx