From 11699b28871460f20d9ef6842d2d24d787c21b4e Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 7 Dec 2021 01:53:19 +0100 Subject: [PATCH 01/14] Add ADR for children as API --- .../adrs/adr-004-children-as-api.md | 288 ++++++++++++++++++ 1 file changed, 288 insertions(+) create mode 100644 contributor-docs/adrs/adr-004-children-as-api.md diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md new file mode 100644 index 00000000000..e2be422ba48 --- /dev/null +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -0,0 +1,288 @@ +# ADR 003: Prop norms in Primer React Components + +## Status + +Proposed + +_Note: These are in no way rules, these should be treated as a starting point for API conversations._ + +## Prefer using children for “content” + +With React, `children` is the out-of-the-box way for putting _content_ inside your component. By using `children` instead our own custom prop, we can make the API “predictable” for its consumers. + +image + +```jsx +// prefer this +Changes saved! +// over this + +``` + +Children as an API for content is an open and composable approach. The contract here is that the `Flash` controls the container while the consumer of the component controls how the contents inside look. + +Take this example of composition: + +flash with icon + +```jsx +import {Flash} from '@primer/react' +import {CheckIcon} from '@primer/octicons-react' + +render( + + Changes saved! + +) +``` + +Pros of this approach here: + +1. The component is aware of recommended use cases and comes with those design decisions backed-in. For example, using an `Icon` with `Flash` is a recognised use case. We don’t lock-in a specific icons, but we do set the size, variant-compatible color and the margin between the icon and text. + + For example: + + flash variants + + ```jsx + + Changes saved! + + + Your changes were not saved! + + ``` + +Continued pros: + +2. You can bring your own icon components, the component does not depend on a specific version of octicons. +3. When a product team wants to explore a use cases which isn’t baked into the component, they are not blocked by our team. + + Example: + + flash with icon and close + + ```jsx + + + Changes saved! + + + +``` + +The icon gets its color and margin based on the variant and size of the `Button`. This is the happy path we want folks to be on, so we ask for the icon component instead of asking the developer to render the icon. + +```jsx +// prefer this: +Search +// over these: + Search +}>Search +``` + +--- + +## Flexibility is a spectrum and the case for composite components + +### There are scenarios where we want to restrict flexibility and bake-in design decisions for the most part, but allow some configuration. + +Consider this fake example: + +image 7 + +We want users to be able to customise if the `Icon` is outline or filled. (I know I know the example is bit silly, but please go with it) + +Extending our `strict` API, we could add another prop to the component: + +```jsx + + Changes saved! + +``` + +When we have an “element” and “elementProp” as props on a component, it’s a sign that we should create a child component here that is tied to the parent component: + +```jsx + + + Changes saved! + +``` + +The `Parent.Child` syntax signals that this component is tied to the parent. + +We can look at `Flash.Icon` as a stricter version of `Icon` that automatically works with different variants of `Flash`. It does not need to support all the props of `Icon` like size or color, only the ones that are compatible in the context of a `Flash`. + +_Note: We might want to name this component `Flash.LeadingIcon`, even if there is no `trailingIcon`. We should try to keep the names consistent across components with the same behavior, but that should not be a deciding factor in making the choice between prop or child component._ + +_Note #2: On the surface it looks we have also removed the `sx` prop for the Icon but because the `sx` prop supports nesting, there is always a way to override styles for better or worse:_ + +```jsx + + Changes saved! + +``` + +--- + +We use this pattern in `NewButton`, `Button.Counter` is a restricted version of `CounterLabel` that automatically adjusts based on the variant and size of a `Button`: + +image 8 + +```jsx + + Watch 1 + + + + Upvote 1 + +``` + +--- + +### Exposing customisation options for internal components: + +Another place where composite patterns lead to aesthetic predictable API is when we want to expose customisation options for internal components: + +image 10 + +```jsx + + Open column menu + ... + +``` + +When we see a a prop which resembles “childProps" on the parent, it's a sign that we could create a composite component: + +```jsx +// we created an additional layer so that +// the overlay props go on the overlay + + Open column menu + + ... + + +``` + +--- + +### Layout components with unstructured content + +In components where there is a place for freeform or unstructured content for the component consumer to fill in, we should prefer the composite components API: + +Consider this fake `Flash` example where description is unstructured content: + +image 11 + +```jsx +// prefer this: + + Changes saved + + These changes will be applied to your next build. Learn more about builds. + + + +// over this: +// Trying to systemise content by finding patterns in +// unconstructured content can lead to overly prescriptive API +// that is not prectictable and hard to remember + +``` + +_Sidenote: It’s tempting to change `icon` to `Flash.Icon` here so that it’s consistent with the rest of the contents. This is a purely aesthetic choice at this point:_ + +```jsx + + + Changes saved + + These changes will be applied to your next build. Learn more about builds. + + +``` + +--- + +We use this pattern in `ActionList v2` : + +actionlist + +```jsx + + + + mona + Monalisa Octocat + + + + primer-css + GitHub + + +``` From edeb948253c1b6efa9592e2c60eff53f4572c613 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 7 Dec 2021 10:53:38 +0100 Subject: [PATCH 02/14] add adrs to eslint ignore list --- .eslintrc.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.eslintrc.json b/.eslintrc.json index 524e372acf2..44a8d95962b 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -22,7 +22,8 @@ "dist/**/*", "lib/**/*", "lib-*/**/*", - "types/**/*" + "types/**/*", + "contributor-docs/adrs/*" ], "globals": { "__DEV__": "readonly" From b1a9ed9cf5458535b3cf500b8ba919dd4c854db2 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 7 Dec 2021 12:06:10 +0100 Subject: [PATCH 03/14] editing phase 1 --- .../adrs/adr-004-children-as-api.md | 111 +++++++++++++----- 1 file changed, 82 insertions(+), 29 deletions(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index e2be422ba48..04eda8b79e9 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -4,8 +4,14 @@ Proposed +
+ _Note: These are in no way rules, these should be treated as a starting point for API conversations._ +_Note #2: Consumer is used multiple times on this page. It refers to the developers consuming the component API and not end users._ + +
+ ## Prefer using children for “content” With React, `children` is the out-of-the-box way for putting _content_ inside your component. By using `children` instead our own custom prop, we can make the API “predictable” for its consumers. @@ -19,6 +25,8 @@ With React, `children` is the out-of-the-box way for putting _content_ inside yo ``` +

+ Children as an API for content is an open and composable approach. The contract here is that the `Flash` controls the container while the consumer of the component controls how the contents inside look. Take this example of composition: @@ -36,7 +44,9 @@ render( ) ``` -Pros of this approach here: +
+ +### Pros of this approach here: 1. The component is aware of recommended use cases and comes with those design decisions backed-in. For example, using an `Icon` with `Flash` is a recognised use case. We don’t lock-in a specific icons, but we do set the size, variant-compatible color and the margin between the icon and text. @@ -53,8 +63,6 @@ Pros of this approach here: ``` -Continued pros: - 2. You can bring your own icon components, the component does not depend on a specific version of octicons. 3. When a product team wants to explore a use cases which isn’t baked into the component, they are not blocked by our team. @@ -71,9 +79,11 @@ Continued pros: ``` +
+ ### Tradeoffs of this approach / When not to use children -Putting all content as in children isn’t a silver bullet. Composition gives flexibility of contents to the consumer, this flexibility however can lead to inconsistencies. +Our goal isn't to put all content inside children though. Composition offers flexibility to the consumer of the component, this flexibility, however, can also lead to inconsistencies. image 6 @@ -88,9 +98,11 @@ Putting all content as in children isn’t a silver bullet. Composition gives fl ``` -_Note: We need to assume good intent here, folks using the components aren’t trying to break the system. They are either trying to implement something that the system’s happy path does not support OR there are multiple ways of doing something with primer and they have unintentionally picked the approach that is not recommended._ +_Note: We need to assume good intent here, developers using the components aren’t trying to break the system. They are either trying to implement something that the system’s happy path does not support OR there are multiple ways of doing something with primer and they have unintentionally picked the approach that is not recommended._ + +
-The general wisdom is to _Make the right (recommended) thing easy to do and the wrong (not recommended) hard to do_. When going off the happy path, developers should feel some friction, some weight, code that “feels wrong” or “feels hacky”. +The general wisdom is to _Make the right (recommended) thing easy to do and the wrong (not recommended) hard to do_. When going off the happy path, developers should feel some friction, some weight, code that “feels hacky” or feels like a workaround. In the above case, if we want to make the recommended path easier and other paths harder, we can change the API to look something like this - @@ -102,8 +114,6 @@ In the above case, if we want to make the recommended path easier and other path We are still using `children` for text content, but we have moved the `icon` back as a prop with reduced flexibility. -\*_Side note: We might want to name this prop `leadingIcon`, even if there is no `trailingIcon`. Consistent names across components plays a big role in making the API predictable._ - When intentionally going off the happy path, developers can still drop down an abstraction level add an `Icon` to `children`, they would have to pick up the additional effort of setting compatible color, size and margin themselves. However, when it’s unintentional, it would feel like way too much work that the component should be doing automatically. ```jsx @@ -113,10 +123,20 @@ When intentionally going off the happy path, developers can still drop down an a ``` +
+ +_Sidenote: We might want to name this prop `leadingIcon`, even if there is no `trailingIcon`. Consistent names across components plays a big role in making the API predictable._ + + + --- +
+ You can see this pattern used in `NewButton`: +The icon gets its color and margin based on the variant and size of the `Button`. This is the happy path we want folks to be on, so we ask for the icon component instead of asking the developer to render the icon. + image 9 ```jsx @@ -124,21 +144,25 @@ You can see this pattern used in `NewButton`: ``` -The icon gets its color and margin based on the variant and size of the `Button`. This is the happy path we want folks to be on, so we ask for the icon component instead of asking the developer to render the icon. - ```jsx -// prefer this: +// we prefer this: Search // over these: Search }>Search ``` +
+ --- +
+ ## Flexibility is a spectrum and the case for composite components -### There are scenarios where we want to restrict flexibility and bake-in design decisions for the most part, but allow some configuration. +
+ +### 1. Scenarios where we want to restrict flexibility and bake-in design decisions for the most part, but allow some configuration. Consider this fake example: @@ -146,6 +170,8 @@ Consider this fake example: We want users to be able to customise if the `Icon` is outline or filled. (I know I know the example is bit silly, but please go with it) +
+ Extending our `strict` API, we could add another prop to the component: ```jsx @@ -163,13 +189,19 @@ When we have an “element” and “elementProp” as props on a component, it ``` +
+ The `Parent.Child` syntax signals that this component is tied to the parent. We can look at `Flash.Icon` as a stricter version of `Icon` that automatically works with different variants of `Flash`. It does not need to support all the props of `Icon` like size or color, only the ones that are compatible in the context of a `Flash`. +It can be tempting to "future proof" our API and adopt this pattern early, but we should resist that until the use case presents itself. _It is always easier to open up the API later than to close it down._ + +
+ _Note: We might want to name this component `Flash.LeadingIcon`, even if there is no `trailingIcon`. We should try to keep the names consistent across components with the same behavior, but that should not be a deciding factor in making the choice between prop or child component._ -_Note #2: On the surface it looks we have also removed the `sx` prop for the Icon but because the `sx` prop supports nesting, there is always a way to override styles for better or worse:_ +_Sidenote: On the surface it looks we have also removed the `sx` prop for the Icon but because the `sx` prop supports nesting, there is always a way to override styles for better or worse:_ ```jsx @@ -177,9 +209,13 @@ _Note #2: On the surface it looks we have also removed the `sx` prop for the Ico ``` +
+ --- -We use this pattern in `NewButton`, `Button.Counter` is a restricted version of `CounterLabel` that automatically adjusts based on the variant and size of a `Button`: +
+ +We use this pattern as well in `NewButton`, `Button.Counter` is a restricted version of `CounterLabel` that automatically adjusts based on the variant and size of a `Button`: image 8 @@ -193,11 +229,17 @@ We use this pattern in `NewButton`, `Button.Counter` is a restricted version of ``` +
+ --- -### Exposing customisation options for internal components: +
+ +### 2. Exposing customisation options for internal components: + +Another place where composite patterns lead to aesthetic predictable APIs is when we want to expose customisation options for internal components. -Another place where composite patterns lead to aesthetic predictable API is when we want to expose customisation options for internal components: +For Example, `ActionMenu` accepts `overlayProps` to pass it down to the implementation detail: image 10 @@ -208,11 +250,13 @@ Another place where composite patterns lead to aesthetic predictable API is when ``` -When we see a a prop which resembles “childProps" on the parent, it's a sign that we could create a composite component: +
+ +When we see a a prop which resembles “childProps" on the container/parent, it's a sign that we should surface this detail in the API by creating a composite component: ```jsx // we created an additional layer so that -// the overlay props go on the overlay +// the overlay props go on the overlay component Open column menu @@ -221,11 +265,15 @@ When we see a a prop which resembles “childProps" on the parent, it's a sign t ``` +
+ --- -### Layout components with unstructured content +
-In components where there is a place for freeform or unstructured content for the component consumer to fill in, we should prefer the composite components API: +### 3. Layout components with unstructured content + +In components where there is a place for consumers to fill in freeform or unstructured content, we should prefer the composite children components. This is especially common in the cases of Dialogs, Menus, Groups. Consider this fake `Flash` example where description is unstructured content: @@ -245,16 +293,17 @@ Consider this fake `Flash` example where description is unstructured content: // unconstructured content can lead to overly prescriptive API // that is not prectictable and hard to remember ``` -_Sidenote: It’s tempting to change `icon` to `Flash.Icon` here so that it’s consistent with the rest of the contents. This is a purely aesthetic choice at this point:_ +
+ +_Sidenote: It’s tempting to change `icon` to `Flash.Icon` here so that it’s consistent with the rest of the contents. This is a purely aesthetic optional choice here:_ ```jsx @@ -266,8 +315,12 @@ _Sidenote: It’s tempting to change `icon` to `Flash.Icon` here so that it’s ``` +
+ --- +
+ We use this pattern in `ActionList v2` : actionlist @@ -275,12 +328,12 @@ We use this pattern in `ActionList v2` : ```jsx - + mona Monalisa Octocat - + primer-css GitHub From 407742813dd11ab12c2a913ae43c18af1f835d2e Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 7 Dec 2021 15:20:54 +0100 Subject: [PATCH 04/14] add NewButton examples --- .../adrs/adr-004-children-as-api.md | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index 04eda8b79e9..65493ab8f26 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -339,3 +339,67 @@ We use this pattern in `ActionList v2` : ``` + +--- + +
+ +### Case study with NewButton: + +image 12 + +Prefer using children for “content” + +```jsx +// we prefer: + +``` + +But, we want to discourage customising the Icon’s color and size in the application. So, in the spirit of making the right thing easy and the wrong thing hard, we ask for the component in a prop instead: + +```jsx +// we prefer: + +// over these: + + +``` + + +image 14 + + +We want to add a `Counter` that adapts to the variant without supporting all the props of a `CounterLabel` like `scheme`. + +`Button.Counter` is a restricted version of `CounterLabel`, making the right thing easy and wrong thing hard: + +```jsx +// we prefer: + +// over this: + + +// it's possible to make a strong case for this option as well: + +``` From 3c3c54e92d213cdb080d116fc547c58a62cdf33c Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 7 Dec 2021 15:47:54 +0100 Subject: [PATCH 05/14] lol title --- contributor-docs/adrs/adr-004-children-as-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index 65493ab8f26..13d0f70ffad 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -1,4 +1,4 @@ -# ADR 003: Prop norms in Primer React Components +# ADR 004: Children as API ## Status From 97812c4dff52229eb0ff762bcee2b1a29a3247f7 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 9 Dec 2021 11:40:53 +0100 Subject: [PATCH 06/14] Replace accidental Button usage --- contributor-docs/adrs/adr-004-children-as-api.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index 13d0f70ffad..974ea129163 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -14,7 +14,7 @@ _Note #2: Consumer is used multiple times on this page. It refers to the develop ## Prefer using children for “content” -With React, `children` is the out-of-the-box way for putting _content_ inside your component. By using `children` instead our own custom prop, we can make the API “predictable” for its consumers. +With React, `children` is the out-of-the-box way for putting _content_ inside your component. By using `children` instead of our own custom prop, we can make the API “predictable” for its consumers. image @@ -140,8 +140,8 @@ The icon gets its color and margin based on the variant and size of the `Button` image 9 ```jsx - - +Search +Search ``` ```jsx From 93e45083d5aa4c54fb6a5cd17358982a7d81ae59 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Fri, 10 Dec 2021 11:09:56 +0100 Subject: [PATCH 07/14] change title --- contributor-docs/adrs/adr-004-children-as-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index 974ea129163..0f28db98807 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -1,4 +1,4 @@ -# ADR 004: Children as API +# ADR 004: Strict props or Composite components ## Status From 6f3671a58823e79f43087a6d830f93a11dfa2d18 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Fri, 10 Dec 2021 14:52:36 +0100 Subject: [PATCH 08/14] add renderChild to the example --- contributor-docs/adrs/adr-004-children-as-api.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index 0f28db98807..f5fd48a1be5 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -239,20 +239,18 @@ We use this pattern as well in `NewButton`, `Button.Counter` is a restricted ver Another place where composite patterns lead to aesthetic predictable APIs is when we want to expose customisation options for internal components. -For Example, `ActionMenu` accepts `overlayProps` to pass it down to the implementation detail: +For Example, `ActionMenu` accepts `overlayProps` and `anchorContent` to pass it down to the implementation details: image 10 ```jsx - - Open column menu - ... + ```
-When we see a a prop which resembles “childProps" on the container/parent, it's a sign that we should surface this detail in the API by creating a composite component: +When we see a a prop which resembles “childProps" or `renderChild` on the container/parent, it's a sign that we should surface this detail in the API by creating a composite component: ```jsx // we created an additional layer so that From a5d9774ee45fb971b499b446552713e8d92ab4ab Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 11 May 2022 15:59:53 +0200 Subject: [PATCH 09/14] Apply suggestions from code review Co-authored-by: Leslie Cohn-Wein Co-authored-by: Cole Bemis --- .../adrs/adr-004-children-as-api.md | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index f5fd48a1be5..d0521082d56 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -83,13 +83,13 @@ render( ### Tradeoffs of this approach / When not to use children -Our goal isn't to put all content inside children though. Composition offers flexibility to the consumer of the component, this flexibility, however, can also lead to inconsistencies. +Our goal isn't to put all content inside children though. Composition offers flexibility to the consumer of the component; this flexibility, however, can also lead to inconsistencies. image 6 ```jsx - // oh oh, we don't know if that color or icon size is the right choice! + // uh oh, we don't know if that color or icon size is the right choice! Changes saved! @@ -98,13 +98,13 @@ Our goal isn't to put all content inside children though. Composition offers fle ``` -_Note: We need to assume good intent here, developers using the components aren’t trying to break the system. They are either trying to implement something that the system’s happy path does not support OR there are multiple ways of doing something with primer and they have unintentionally picked the approach that is not recommended._ +_Note: We need to assume good intent here, developers using the components aren’t trying to break the system. They are either trying to implement something that the system’s happy path does not support OR there are multiple ways of doing something with Primer and they have unintentionally picked the approach that is not recommended._
The general wisdom is to _Make the right (recommended) thing easy to do and the wrong (not recommended) hard to do_. When going off the happy path, developers should feel some friction, some weight, code that “feels hacky” or feels like a workaround. -In the above case, if we want to make the recommended path easier and other paths harder, we can change the API to look something like this - +In the above case, if we want to make the recommended path easier and other paths harder, we can change the API to look something like this: ```jsx @@ -114,7 +114,7 @@ In the above case, if we want to make the recommended path easier and other path We are still using `children` for text content, but we have moved the `icon` back as a prop with reduced flexibility. -When intentionally going off the happy path, developers can still drop down an abstraction level add an `Icon` to `children`, they would have to pick up the additional effort of setting compatible color, size and margin themselves. However, when it’s unintentional, it would feel like way too much work that the component should be doing automatically. +When intentionally going off the happy path, developers can still drop down an abstraction level to add an `Icon` to `children`, though they would have to pick up the additional effort of setting compatible color, size and margin themselves. However, when it’s unintentional, it would feel like way too much work that the component should be doing automatically. ```jsx @@ -215,19 +215,18 @@ _Sidenote: On the surface it looks we have also removed the `sx` prop for the Ic
-We use this pattern as well in `NewButton`, `Button.Counter` is a restricted version of `CounterLabel` that automatically adjusts based on the variant and size of a `Button`: +We use this pattern as well in `Button`, `Button.Counter` is a restricted version of `CounterLabel` that automatically adjusts based on the variant and size of a `Button`: image 8 ```jsx - - Watch 1 - + - - Upvote 1 - -``` +
@@ -319,7 +318,7 @@ _Sidenote: It’s tempting to change `icon` to `Flash.Icon` here so that it’s
-We use this pattern in `ActionList v2` : +We use this pattern in `ActionList` : actionlist @@ -342,7 +341,7 @@ We use this pattern in `ActionList v2` :
-### Case study with NewButton: +### Case study with Button: image 12 @@ -377,7 +376,7 @@ But, we want to discourage customising the Icon’s color and size in the applic // over these: - + ``` From c184b9efcaa25217479fb909a3e3e97442a3ccd1 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 25 May 2022 15:00:40 +0200 Subject: [PATCH 10/14] Apply suggestions from code review Co-authored-by: Cole Bemis --- contributor-docs/adrs/adr-004-children-as-api.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index d0521082d56..59c4dec970c 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -2,7 +2,7 @@ ## Status -Proposed +Approved 2022-05-10
@@ -133,23 +133,23 @@ _Sidenote: We might want to name this prop `leadingIcon`, even if there is no `t
-You can see this pattern used in `NewButton`: +You can see this pattern used in `Button`: The icon gets its color and margin based on the variant and size of the `Button`. This is the happy path we want folks to be on, so we ask for the icon component instead of asking the developer to render the icon. image 9 ```jsx -Search -Search + + ``` ```jsx // we prefer this: -Search + // over these: - Search -}>Search + + +``` + +On the other hand, if we want consumers to have more control over children, a composite API is the better choice. + +```jsx + + + + + mona + +``` + ## Prefer using children for “content” With React, `children` is the out-of-the-box way for putting _content_ inside your component. By using `children` instead of our own custom prop, we can make the API “predictable” for its consumers. From 8be9110df7ec22c8bed1f5e5c551a9cc3da125fd Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 25 May 2022 15:21:11 +0200 Subject: [PATCH 12/14] Update contributor-docs/adrs/adr-004-children-as-api.md --- contributor-docs/adrs/adr-004-children-as-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index c8ddb367c75..33ac24a712b 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -36,7 +36,7 @@ On the other hand, if we want consumers to have more control over children, a co ## Prefer using children for “content” -With React, `children` is the out-of-the-box way for putting _content_ inside your component. By using `children` instead of our own custom prop, we can make the API “predictable” for its consumers. +With React, `children` is the out-of-the-box way for putting [phrasing content](https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#phrasing_content) inside your component. By using `children` instead of our own custom prop, we can make the API “predictable” for its consumers. image From c1f2d25215089c6e0ffdc5f28c384be05889c233 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 25 May 2022 15:23:21 +0200 Subject: [PATCH 13/14] clarify ActionMenu example is from legacy version --- contributor-docs/adrs/adr-004-children-as-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index 33ac24a712b..84596f334fc 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -260,7 +260,7 @@ We use this pattern as well in `Button`, `Button.Counter` is a restricted versio Another place where composite patterns lead to aesthetic predictable APIs is when we want to expose customisation options for internal components. -For Example, `ActionMenu` accepts `overlayProps` and `anchorContent` to pass it down to the implementation details: +For Example, [legacy ActionMenu](https://primer.style/react/deprecated/ActionMenu) accepted `overlayProps` and `anchorContent` to pass it down to the implementation details: image 10 From 8e7e45ebd24533fb19fb93162fef1b8528c27ffc Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 25 May 2022 15:25:08 +0200 Subject: [PATCH 14/14] removed sidenote because it feels like a tangent --- contributor-docs/adrs/adr-004-children-as-api.md | 8 -------- 1 file changed, 8 deletions(-) diff --git a/contributor-docs/adrs/adr-004-children-as-api.md b/contributor-docs/adrs/adr-004-children-as-api.md index 84596f334fc..0dff7e8f0e8 100644 --- a/contributor-docs/adrs/adr-004-children-as-api.md +++ b/contributor-docs/adrs/adr-004-children-as-api.md @@ -223,14 +223,6 @@ It can be tempting to "future proof" our API and adopt this pattern early, but w _Note: We might want to name this component `Flash.LeadingIcon`, even if there is no `trailingIcon`. We should try to keep the names consistent across components with the same behavior, but that should not be a deciding factor in making the choice between prop or child component._ -_Sidenote: On the surface it looks we have also removed the `sx` prop for the Icon but because the `sx` prop supports nesting, there is always a way to override styles for better or worse:_ - -```jsx - - Changes saved! - -``` -
---