fix(Buttons): correctly support overriding default `data-garden-id` attribute #1804

ze-flo · 2024-05-09T04:34:51Z

Description

~~### ❗❗ BREAKING CHANGES ❗❗~~

~~Users targeting Button and IconButton using the data-garden-id attribute (commonly for testing purposes) should acknowledge these id changes:~~

See previous breaking changes

Module	Component	Previous ID	New ID
`@zendeskgarden/react-notifications`	`GlobalAlert.Close`	`buttons.button`	`notifications.global-alert.button`
`@zendeskgarden/react-colorpickers`	`ColorSwatchDialog`	`buttons.button`	`colorpickers.colordialog_button`
`@zendeskgarden/react-dropdowns` (formerly `react-dropdowns.next`)	`Menu`	`buttons.button`	`dropdowns.menu.button`
`@zendeskgarden/react-grid`	`Pane.SplitterButton`	`buttons.icon_button`	`pane.splitter_button`
`@zendeskgarden/react-notifications`	`GlobalAlert`	`buttons.icon_button`	`notifications.global-alert.close`

~~We can avoid the breaking change by renaming the new ids to match the previous ones.~~

Detail

Fix support for overriding default data-garden-id for each button component exported from @zendeskgarden/react-buttons

Example

react-components/packages/notifications/src/styled/global-alert/StyledGlobalAlertButton.ts

Line 20 in 717a8d5

const COMPONENT_ID = 'notifications.global-alert.button';

react-components/packages/notifications/src/styled/global-alert/StyledGlobalAlertButton.ts

Lines 92 to 100 in 717a8d5

    
           export const StyledGlobalAlertButton = styled(Button).attrs({ 
        
             'data-garden-id': COMPONENT_ID, 
        
             'data-garden-version': PACKAGE_VERSION, 
        
             focusInset: false, 
        
             isDanger: false, 
        
             isLink: false, 
        
             isNeutral: false, 
        
             isPill: false,

Checklist

~~👌 design updates will be Garden Designer approved (add the designer as a reviewer)~~
🌐 demo is up-to-date (npm start)
~~⬅️ renders as expected with reversed (RTL) direction~~
~~🤘 renders as expected with Bedrock CSS (?bedrock)~~
💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
~~♿ tested for WCAG 2.1 AA accessibility compliance~~
📝 tested in Chrome, Firefox, Safari, and Edge

…ntId`

ze-flo · 2024-05-09T04:49:12Z

packages/buttons/src/styled/StyledAnchor.ts

@@ -24,7 +25,7 @@ export const StyledAnchor = styled(StyledButton).attrs(props => ({
 }))`
  direction: ${props => props.theme.rtl && 'rtl'};

-  ${props => retrieveComponentStyles(COMPONENT_ID, props)};
+  ${props => retrieveComponentStyles(props['data-garden-id'], props)};


props always include the ones set in .attrs. If we complete this type of migration for every component and drop the componentId param from `retrieveComponentStyles, we could rewrite this to:

Suggested change

${props => retrieveComponentStyles(props['data-garden-id'], props)};

${retrieveComponentStyles};

I would discourage the rewrite at this point since a v10 goal is to consolidate packages. At that point, we should visit styled component re-wrapping in earnest and make sure we have the right solution for CSS targeting.

jzempel

I don't think this is necessary. Please compare menu with combobox in the dropdowns.next package. While it doesn't always get sorted in dev, the attrs work out as expected in production.

ze-flo · 2024-05-15T21:02:38Z

packages/buttons/src/styled/StyledIconButton.ts

+export const StyledIconButton = styled(StyledButton).attrs<IButtonProps>(props => ({
+  'data-garden-id': props['data-garden-id'] || COMPONENT_ID,


As mentioned in this comment, this pattern causes a regression with IconButton. StyledIconButton now inherits the COMPONENT_ID buttons.button from StyledButton. It should be:

react-components/packages/buttons/src/styled/StyledIconButton.ts

Line 14 in 5282298

const COMPONENT_ID = 'buttons.icon_button';

For styled-components extending another styled-component, we must first check the data-garden-id doesn't match the one set on the styled-component it extends.

Suggested change

export const StyledIconButton = styled(StyledButton).attrs<IButtonProps>(props => ({

'data-garden-id': props['data-garden-id'] || COMPONENT_ID,

export const StyledIconButton = styled(StyledButton).attrs<IButtonProps>(props => ({

'data-garden-id':

props['data-garden-id'] && props['data-garden-id'] !== 'buttons.button'

? props['data-garden-id']

: COMPONENT_ID,

This sandbox highlights all the possible combinations, what works and what doesn't. There's a lot there to unfold. You've been warned. 😅

…component

coveralls · 2024-05-15T23:02:53Z

coverage: 96.201% (-0.02%) from 96.216%
when pulling 608afe0 on ze-flo/button-override-attrs
into 717a8d5 on next.

ze-flo · 2024-05-15T23:24:33Z

Summary

Here are 2 rules-of-thumb when creating Styled components with data-garden-id:

styled-components that apply styles to a HTML element should uses the attrs Prop Factory fn to support overriding the default data-garden-id attribute if it receives one.
styled-components that extend another styled-component should:
a. Use the attrs 'Prop Factory' fn to support overriding the default data-garden-id attribute if it receives one.
b. AND first checks that the received data-garden-id doesn't match the one set on the styled-component it extends.

const BASIC_BTN_ID = 'basic-btn';
/**
 * styled-component that apply styles to a HTML element should
 * uses the `attrs` 'Prop Factory' fn to support overriding
 * the default `data-garden-id` attribute if it receives one.
 * @see https://styled-components.com/docs/api#.attrs
 * 
 */
const StyledBasicBtn = styled.button.attrs((props) => ({
  'data-garden-id': props['data-garden-id'] || BASIC_BTN_ID
}))`
  ...
`;

CONST PINK_BTN_ID = 'pink-btn';

/**
 * styled-components that extend another styled-component should:
 * 1. use the `attrs` 'Prop Factory' fn to support overriding
 *    the default `data-garden-id` attribute if it receives one.
 * 2. First checks that the received `data-garden-id`
 *    doesn't match the one set on the styled-component it extends.
 */
const StyledPinkBtn = styled(StyledBasicBtn).attrs((props) => ({
  'data-garden-id':
    props['data-garden-id'] && props['data-garden-id'] !== BASIC_BTN_ID // Very important!
      ? props['data-garden-id']
      : PINK_BTN_ID
}))`
  background: hotpink;
`;

This sandbox highlights all the possible combinations, what works , and what doesn't.

jzempel

needs migration.md update similar to 3464347
consider using simple data-garden-id comparison strings and avoiding the added export/import clunkiness (i.e. toHaveAttribute('data-garden-id', 'pane.splitter_button'))

In general, let's really try to limit the spread here. I don't think we want this pattern to be normative. The backing and overarching goal is simply to retain the modal.close component ID while being able to reuse Button 😅

jzempel · 2024-05-16T13:53:56Z

packages/buttons/src/styled/StyledAnchor.ts

+export const StyledAnchor = styled(StyledButton).attrs<IAnchorProps>(props => ({
+  'data-garden-id':
+    props['data-garden-id'] && props['data-garden-id'] !== BTN_COMPONENT_ID
+      ? props['data-garden-id']
+      : COMPONENT_ID,


Suggested change

export const StyledAnchor = styled(StyledButton).attrs<IAnchorProps>(props => ({

'data-garden-id':

props['data-garden-id'] && props['data-garden-id'] !== BTN_COMPONENT_ID

? props['data-garden-id']

: COMPONENT_ID,

export const StyledAnchor = styled(StyledButton).attrs<IAnchorProps>(props => {

const externalId = props['data-garden-id'];

const componentId = externalId && externalId !== BTN_COMPONENT_ID ? externalId : COMPONENT_ID;

return {

'data-garden-id': componentId,

would this improve readability?

jzempel · 2024-05-16T14:11:33Z

packages/buttons/src/styled/StyledIconButton.ts

+export const StyledIconButton = styled(StyledButton).attrs<IButtonProps>(props => ({
+  'data-garden-id':
+    props['data-garden-id'] && props['data-garden-id'] !== BTN_COMPONENT_ID
+      ? props['data-garden-id']
+      : COMPONENT_ID,


ditto re: readability

jzempel · 2024-05-16T14:14:04Z

packages/buttons/src/styled/StyledSplitButton.ts

-export const StyledSplitButton = styled.div.attrs({
-  'data-garden-id': COMPONENT_ID,
+export const StyledSplitButton = styled.div.attrs<ISplitButtonProps>(props => ({
+  'data-garden-id': props['data-garden-id'] || COMPONENT_ID,


Is this necessary? Where is SplitButton wrapped? We should limit the spread of this change as much as possible.

jzempel · 2024-05-16T14:15:17Z

packages/buttons/src/styled/StyledIcon.ts

-export const StyledIcon = styled(StyledBaseIcon).attrs({
-  'data-garden-id': COMPONENT_ID,
+export const StyledIcon = styled(StyledBaseIcon).attrs<IStyledIconProps>(props => ({
+  'data-garden-id': props['data-garden-id'] || COMPONENT_ID,


ditto along with SplitButton – is this necessary?

jzempel · 2024-05-16T14:17:22Z

packages/buttons/src/types/index.ts

+  /** @ignore Set identifier to retrieve component styles */
+  'data-garden-id'?: string;


I think we can prefer limited (props as any)['data-garden-id'] vs. any updates to types.

jzempel · 2024-05-16T14:18:50Z

packages/buttons/src/styled/index.ts

+export { COMPONENT_ID as BTN_COMPONENT_ID, StyledButton } from './StyledButton';
 export * from './StyledSplitButton';
 export * from './StyledExternalIcon';
-export * from './StyledIcon';
-export * from './StyledIconButton';
+export { COMPONENT_ID as ICON_COMPONENT_ID, StyledIcon } from './StyledIcon';
+export { COMPONENT_ID as ICON_BTN_COMPONENT_ID, StyledIconButton } from './StyledIconButton';


Are these COMPONENT_ID exports necessary?

jzempel · 2024-05-16T14:24:27Z

packages/dropdowns/src/elements/menu/Menu.tsx

-        <StyledButton {...triggerProps}>
+        <Button {...triggerProps} data-garden-id={BTN_COMPONENT_ID}>
          {button}
-          <StyledButton.EndIcon isRotated={isExpanded}>
+          <Button.EndIcon isRotated={isExpanded}>
            <ChevronIcon />
-          </StyledButton.EndIcon>
-        </StyledButton>
+          </Button.EndIcon>
+        </Button>


Let's keep StyledButton restored rather than making architecture assertions in this PR.

jzempel · 2024-05-16T14:30:51Z

packages/theming/src/utils/retrieveComponentStyles.ts

@@ -9,12 +9,12 @@ import { ThemeProps, DefaultTheme } from 'styled-components';

 /** @component */
 export default function retrieveComponentStyles(
-  componentId: string,
+  componentId: string | undefined,


We shouldn't allow undefined here.

This reverts commit 2527990. # Conflicts: # packages/dropdowns/src/elements/menu/Menu.tsx

…`componentId`" This reverts commit ec71f03.

… ze-flo/button-override-attrs

ze-flo · 2024-05-16T20:10:29Z

packages/dropdowns/src/views/menu/StyledButton.ts

-export const StyledButton = styled(Button).attrs({
-  'data-garden-id': COMPONENT_ID,
-  'data-garden-version': PACKAGE_VERSION
-})`
-  ${props => retrieveComponentStyles(COMPONENT_ID, props)};
-`;


RE: #1804 (comment)

After removing all code causing a breaking-change, we would be left with:

export const StyledButton = styled(Button)``

So I think it's better to delete.

ze-flo · 2024-05-16T20:41:07Z

Verified that breaking-changes have been removed and that the data-garden-id matches with V8 for the following components:

This also aligns with updated specs.

jzempel · 2024-05-17T14:46:55Z

packages/buttons/src/styled/StyledAnchor.ts

Maybe I'm missing something, but I'm not seeing the need for changes to StyledAnchor in this PR.

I was applying the new dev pattern consistently to all Styled buttons that could be extended. At this time, however, neither StyledAnchor nor Anchor are extended anywhere else.

Should I rollback?

Yes, please. Again, let's keep this somewhat hack as minimal as possible so that we have less to unwind when package consolidation is a thing.

jzempel · 2024-05-17T14:52:32Z

packages/buttons/src/styled/StyledButton.ts

Can this rollback now that there are no more competing styled(Button) extensions? The remaining styled(StyledButton) extension still works as expected without this, right?

If I'm understanding your questions correctly, rolling back would create a breaking-change with the Modal.Close button in #1808. As you can see below, the data-garden-id will be buttons.icon_button rather than modals.close.

demo.mp4

You can see this in action by updating line 51 in the Sandbox.

Ah, got it. Thanks for the clarification/reminder. Sorry, my brain is slow on accepting this (anti)pattern 😜

The crazy magic built into styled-components took me a while to grasp and is forcing us down a less-than-ideal path. I definitely wanna revisit this in the near future to find a better solution.

ze-flo added 3 commits May 8, 2024 16:53

chore(retrieveComponentStyles): graciously support missing `compone…

ec71f03

…ntId`

fix(Buttons): support overriding data-garden-id

7b3082e

refactor(Menu): replace with `Button

2527990

ze-flo added the PR: Breaking Change 💥 label May 9, 2024

ze-flo requested a review from a team as a code owner May 9, 2024 04:34

ze-flo changed the base branch from main to next May 9, 2024 04:44

ze-flo commented May 9, 2024

View reviewed changes

jzempel requested changes May 9, 2024

View reviewed changes

This was referenced May 14, 2024

feat(Modal): new light/dark mode colors #1808

Merged

feat(buttons): new light/dark mode colors #1807

Merged

ze-flo commented May 15, 2024

View reviewed changes

ze-flo added 3 commits May 15, 2024 11:31

fix(Buttons): remove data-garden-id regression when extending styled-…

93dfd81

…component

test: add specs for data-garden-id

47c49b3

chore(Buttons): fix linting error

de89248

zendesk-garden deployed to staging May 15, 2024 23:03 View deployment

jzempel requested changes May 16, 2024

View reviewed changes

ze-flo added 6 commits May 16, 2024 08:15

chore: Revert "fix(Buttons): support overriding data-garden-id"

d595039

chore: PR feedback

113b75b

Revert "refactor(Menu): replace with `Button"

fedefc6

This reverts commit 2527990. # Conflicts: # packages/dropdowns/src/elements/menu/Menu.tsx

Revert "chore(retrieveComponentStyles): graciously support missing …

18170d9

…`componentId`" This reverts commit ec71f03.

refactor: remove breaking changes

604874d

chore: fix type

608afe0

zendesk-garden deployed to staging May 16, 2024 20:23 View deployment

Merge branch 'next' of github.com:zendeskgarden/react-components into…

fa018a5

… ze-flo/button-override-attrs

ze-flo changed the title ~~fix!(Buttons): correctly support overriding default data-garden-id attribute~~ fix(Buttons): correctly support overriding default data-garden-id attribute May 16, 2024

ze-flo added PR: Internal 🌱 and removed PR: Breaking Change 💥 labels May 16, 2024

ze-flo commented May 16, 2024

View reviewed changes

zendesk-garden deployed to staging May 16, 2024 20:43 View deployment

jzempel reviewed May 17, 2024

View reviewed changes

chore: revert unnecessary changes

f64953f

zendesk-garden deployed to staging May 17, 2024 19:35 View deployment

jzempel approved these changes May 17, 2024

View reviewed changes

ze-flo merged commit 12c7917 into next May 17, 2024
5 checks passed

ze-flo deleted the ze-flo/button-override-attrs branch May 17, 2024 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Buttons): correctly support overriding default `data-garden-id` attribute #1804

fix(Buttons): correctly support overriding default `data-garden-id` attribute #1804

ze-flo commented May 9, 2024 •

edited

ze-flo May 9, 2024 •

edited

jzempel May 16, 2024

jzempel left a comment

ze-flo May 15, 2024

coveralls commented May 15, 2024 •

edited

ze-flo commented May 15, 2024 •

edited

jzempel left a comment

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

ze-flo May 16, 2024

ze-flo commented May 16, 2024 •

edited

jzempel May 17, 2024

ze-flo May 17, 2024

jzempel May 17, 2024

jzempel May 17, 2024

ze-flo May 17, 2024

jzempel May 17, 2024

ze-flo May 17, 2024


	export const StyledGlobalAlertButton = styled(Button).attrs({
	'data-garden-id': COMPONENT_ID,
	'data-garden-version': PACKAGE_VERSION,
	focusInset: false,
	isDanger: false,
	isLink: false,
	isNeutral: false,
	isPill: false,

	${props => retrieveComponentStyles(props['data-garden-id'], props)};
	${retrieveComponentStyles};

		export const StyledIconButton = styled(StyledButton).attrs<IButtonProps>(props => ({
		'data-garden-id': props['data-garden-id'] \|\| COMPONENT_ID,

		/** @ignore Set identifier to retrieve component styles */
		'data-garden-id'?: string;

fix(Buttons): correctly support overriding default data-garden-id attribute #1804

fix(Buttons): correctly support overriding default data-garden-id attribute #1804

Conversation

ze-flo commented May 9, 2024 • edited

Description

Detail

Example

Checklist

ze-flo May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzempel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 15, 2024 • edited

ze-flo commented May 15, 2024 • edited

Summary

jzempel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ze-flo commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix(Buttons): correctly support overriding default `data-garden-id` attribute #1804

fix(Buttons): correctly support overriding default `data-garden-id` attribute #1804

ze-flo commented May 9, 2024 •

edited

ze-flo May 9, 2024 •

edited

coveralls commented May 15, 2024 •

edited

ze-flo commented May 15, 2024 •

edited

ze-flo commented May 16, 2024 •

edited