Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
May 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ec71f03
chore(`retrieveComponentStyles`): graciously support missing `compone…
ze-flo May 9, 2024
7b3082e
fix(Buttons): support overriding `data-garden-id`
ze-flo May 9, 2024
2527990
refactor(Menu): replace with `Button
ze-flo May 9, 2024
93dfd81
fix(Buttons): remove data-garden-id regression when extending styled-…
ze-flo May 15, 2024
47c49b3
test: add specs for `data-garden-id`
ze-flo May 15, 2024
de89248
chore(Buttons): fix linting error
ze-flo May 15, 2024
d595039
chore: Revert "fix(Buttons): support overriding `data-garden-id`"
ze-flo May 16, 2024
113b75b
chore: PR feedback
ze-flo May 16, 2024
fedefc6
Revert "refactor(Menu): replace with `Button"
ze-flo May 16, 2024
18170d9
Revert "chore(`retrieveComponentStyles`): graciously support missing …
ze-flo May 16, 2024
604874d
refactor: remove breaking changes
ze-flo May 16, 2024
608afe0
chore: fix type
ze-flo May 16, 2024
fa018a5
Merge branch 'next' of github.com:zendeskgarden/react-components into…
ze-flo May 16, 2024
f64953f
chore: revert unnecessary changes
ze-flo May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/buttons/src/elements/Button.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React from 'react';
import { render, renderRtl } from 'garden-test-utils';
import { Button } from './Button';
import TestIcon from '@zendeskgarden/svg-icons/src/16/gear-stroke.svg';
import { COMPONENT_ID } from '../styled/StyledButton';

describe('Button', () => {
it('is rendered as a button', () => {
Expand All @@ -24,6 +25,20 @@ describe('Button', () => {
expect(container.firstChild).toBe(ref.current);
});

describe('`data-garden-id` attribute', () => {
it('sets a default data-garden-id attribute', () => {
const { getByRole } = render(<Button />);

expect(getByRole('button')).toHaveAttribute('data-garden-id', COMPONENT_ID);
});

it('supports overriding the data-garden-id attribute', () => {
const { getByRole } = render(<Button data-garden-id="myCoolButton" />);

expect(getByRole('button')).toHaveAttribute('data-garden-id', 'myCoolButton');
});
});

describe('Icons', () => {
it('successfully renders start and end icons', () => {
const { getByTestId } = render(
Expand Down
10 changes: 9 additions & 1 deletion packages/buttons/src/styled/StyledAnchor.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import React from 'react';
import { render, renderRtl } from 'garden-test-utils';
import { PALETTE } from '@zendeskgarden/react-theming';
import { StyledAnchor } from './StyledAnchor';
import { COMPONENT_ID, StyledAnchor } from './StyledAnchor';

describe('StyledAnchor', () => {
it('renders the expected element', () => {
Expand All @@ -34,4 +34,12 @@ describe('StyledAnchor', () => {

expect(container.firstChild).toHaveStyleRule('direction', 'rtl');
});

describe('`data-garden-id` attribute', () => {
it('has the correct `data-garden-id`', () => {
const { container } = render(<StyledAnchor />);

expect(container.firstChild).toHaveAttribute('data-garden-id', COMPONENT_ID);
});
});
});
27 changes: 16 additions & 11 deletions packages/buttons/src/styled/StyledAnchor.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,29 @@

import styled from 'styled-components';
import { retrieveComponentStyles, DEFAULT_THEME } from '@zendeskgarden/react-theming';
import { StyledButton } from './StyledButton';
import { COMPONENT_ID as BTN_COMPONENT_ID, StyledButton } from './StyledButton';
import { IAnchorProps } from './../types';

const COMPONENT_ID = 'buttons.anchor';
export const COMPONENT_ID = 'buttons.anchor';

/**
* Accepts all `<a>` props
*/
export const StyledAnchor = styled(StyledButton).attrs(props => ({
'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION,
as: 'a',
dir: props.theme.rtl ? 'rtl' : undefined,
isLink: true,
type: undefined
}))`
export const StyledAnchor = styled(StyledButton).attrs<IAnchorProps>(props => {
const externalId: string = (props as any)['data-garden-id'];

return {
'data-garden-id': externalId && externalId !== BTN_COMPONENT_ID ? externalId : COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION,
as: 'a',
dir: props.theme.rtl ? 'rtl' : undefined,
isLink: true,
type: undefined
};
})`
direction: ${props => props.theme.rtl && 'rtl'};

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

StyledAnchor.defaultProps = {
Expand Down
4 changes: 2 additions & 2 deletions packages/buttons/src/styled/StyledButton.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screen Recording 2024-05-17 at 5 41 52 AM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { IButtonProps } from '../types';
import { StyledSplitButton } from './StyledSplitButton';
import { StyledIcon } from './StyledIcon';

const COMPONENT_ID = 'buttons.button';
export const COMPONENT_ID = 'buttons.button';

const getBorderRadius = (props: IButtonProps & ThemeProps<DefaultTheme>) => {
if (props.isPill) {
Expand Down Expand Up @@ -431,7 +431,7 @@ const sizeStyles = (props: IButtonProps & ThemeProps<DefaultTheme>) => {
* 3. Shifting :focus-visible from LVHFA order to preserve `text-decoration` on hover
*/
export const StyledButton = styled.button.attrs<IButtonProps>(props => ({
'data-garden-id': COMPONENT_ID,
'data-garden-id': (props as any)['data-garden-id'] || COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION,
type: props.type || 'button'
}))<IButtonProps>`
Expand Down
14 changes: 13 additions & 1 deletion packages/buttons/src/styled/StyledIcon.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import React from 'react';
import { render, renderRtl } from 'garden-test-utils';
import { StyledIcon } from './StyledIcon';
import { COMPONENT_ID, StyledIcon } from './StyledIcon';
import TestIcon from '@zendeskgarden/svg-icons/src/16/gear-stroke.svg';

describe('StyledIcon', () => {
Expand Down Expand Up @@ -79,4 +79,16 @@ describe('StyledIcon', () => {
}).toThrow();
});
});

describe('`data-garden-id` attribute', () => {
it('has the correct `data-garden-id`', () => {
const { container } = render(
<StyledIcon>
<TestIcon />
</StyledIcon>
);

expect(container.firstChild).toHaveAttribute('data-garden-id', COMPONENT_ID);
});
});
});
2 changes: 1 addition & 1 deletion packages/buttons/src/styled/StyledIcon.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
retrieveComponentStyles
} from '@zendeskgarden/react-theming';

const COMPONENT_ID = 'buttons.icon';
export const COMPONENT_ID = 'buttons.icon';

interface IStyledIconProps {
$isRotated: boolean;
Expand Down
10 changes: 9 additions & 1 deletion packages/buttons/src/styled/StyledIconButton.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import React from 'react';
import { css } from 'styled-components';
import { render } from 'garden-test-utils';
import { StyledIconButton } from './StyledIconButton';
import { COMPONENT_ID, StyledIconButton } from './StyledIconButton';
import { StyledIcon } from './StyledIcon';
import { PALETTE } from '@zendeskgarden/react-theming';
import { rgba } from 'polished';
Expand Down Expand Up @@ -88,4 +88,12 @@ describe('StyledIconButton', () => {
});
});
});

describe('`data-garden-id` attribute', () => {
it('has the correct `data-garden-id`', () => {
const { container } = render(<StyledIconButton />);

expect(container.firstChild).toHaveAttribute('data-garden-id', COMPONENT_ID);
});
});
});
18 changes: 11 additions & 7 deletions packages/buttons/src/styled/StyledIconButton.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
import styled, { css, ThemeProps, DefaultTheme } from 'styled-components';
import { retrieveComponentStyles, DEFAULT_THEME, getColor } from '@zendeskgarden/react-theming';
import { IButtonProps } from '../types';
import { StyledButton, getHeight } from './StyledButton';
import { COMPONENT_ID as BTN_COMPONENT_ID, StyledButton, getHeight } from './StyledButton';
import { StyledIcon } from './StyledIcon';

const COMPONENT_ID = 'buttons.icon_button';
export const COMPONENT_ID = 'buttons.icon_button';

const iconColorStyles = ({ theme }: IButtonProps & ThemeProps<DefaultTheme>) => {
const options = { theme, variable: 'foreground.default' };
Expand Down Expand Up @@ -69,17 +69,21 @@ const iconStyles = (props: IButtonProps & ThemeProps<DefaultTheme>) => {
`;
};

export const StyledIconButton = styled(StyledButton as 'button').attrs({
'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION
})<IButtonProps>`
export const StyledIconButton = styled(StyledButton).attrs<IButtonProps>(props => {
const externalId: string = (props as any)['data-garden-id'];

return {
'data-garden-id': externalId && externalId !== BTN_COMPONENT_ID ? externalId : COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION
};
})`
${props => iconButtonStyles(props)};

& ${StyledIcon} {
${props => iconStyles(props)}
}

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

StyledIconButton.defaultProps = {
Expand Down
6 changes: 3 additions & 3 deletions packages/buttons/src/styled/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
*/

export * from './StyledAnchor';
export * from './StyledButton';
export { StyledButton } from './StyledButton';
export * from './StyledSplitButton';
export * from './StyledExternalIcon';
export * from './StyledIcon';
export * from './StyledIconButton';
export { StyledIcon } from './StyledIcon';
export { StyledIconButton } from './StyledIconButton';
16 changes: 16 additions & 0 deletions packages/colorpickers/src/elements/ColorSwatchDialog/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,20 @@ describe('ColorSwatchDialog', () => {
expect(dialog).toBeNull();
});
});

describe('`data-garden-id` attribute', () => {
it('has the correct `data-garden-id`', () => {
const { getByRole } = render(
<ColorSwatchDialog
name="test"
colors={colors}
buttonProps={{
'aria-label': 'Choose your favorite color'
}}
/>
);

expect(getByRole('button')).toHaveAttribute('data-garden-id', 'buttons.button');
});
});
});
7 changes: 1 addition & 6 deletions packages/colorpickers/src/styled/ColorPickerDialog/StyledButton.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,15 @@
*/

import styled from 'styled-components';
import { retrieveComponentStyles, DEFAULT_THEME } from '@zendeskgarden/react-theming';
import { DEFAULT_THEME } from '@zendeskgarden/react-theming';
import { Button } from '@zendeskgarden/react-buttons';

const COMPONENT_ID = 'colorpickers.colordialog_button';

/**
* 1. IE11 group width override.
* 2. Input group border overrides.
*/
export const StyledButton = styled(Button as any).attrs({
isNeutral: true,
'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION
})`
padding: 0;
Expand All @@ -32,8 +29,6 @@ export const StyledButton = styled(Button as any).attrs({
${props => props.theme.borderRadii.md} !important; /* [2] */
/* stylelint-enable */
}

${props => retrieveComponentStyles(COMPONENT_ID, props)};
`;

StyledButton.defaultProps = {
Expand Down
11 changes: 11 additions & 0 deletions packages/dropdowns/src/elements/menu/Menu.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,4 +659,15 @@ describe('Menu', () => {
expect(getByTestId('grape')).toHaveAttribute('aria-checked', 'true');
});
});

describe('`data-garden-id` attribute', () => {
it('has the correct `data-garden-id`', async () => {
const { getByRole } = render(<TestMenu button="click me" />);

await floating();
const button = getByRole('button');

expect(button).toHaveAttribute('data-garden-id', 'buttons.button');
});
});
});
11 changes: 5 additions & 6 deletions packages/dropdowns/src/elements/menu/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,11 @@ import { mergeRefs } from 'react-merge-refs';
import { ThemeContext } from 'styled-components';
import { useMenu } from '@zendeskgarden/container-menu';
import { DEFAULT_THEME, useWindow } from '@zendeskgarden/react-theming';
import { IButtonProps } from '@zendeskgarden/react-buttons';
import { Button, IButtonProps } from '@zendeskgarden/react-buttons';
import { IMenuProps, PLACEMENT } from '../../types';
import { MenuContext } from '../../context/useMenuContext';
import { toItems } from './utils';
import { MenuList } from './MenuList';
import { StyledButton } from '../../views';
import ChevronIcon from '@zendeskgarden/svg-icons/src/16/chevron-down-stroke.svg';

/**
Expand Down Expand Up @@ -87,12 +86,12 @@ export const Menu = forwardRef<HTMLUListElement, IMenuProps>(
typeof button === 'function' ? (
button(triggerProps)
) : (
<StyledButton {...triggerProps}>
<Button {...triggerProps}>
{button}
<StyledButton.EndIcon isRotated={isExpanded}>
<Button.EndIcon isRotated={isExpanded}>
<ChevronIcon />
</StyledButton.EndIcon>
</StyledButton>
</Button.EndIcon>
</Button>
);

const contextValue = useMemo(
Expand Down
1 change: 0 additions & 1 deletion packages/dropdowns/src/views/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,4 @@ export * from './menu/StyledItemGroup';
export * from './menu/StyledItemIcon';
export * from './menu/StyledItemMeta';
export * from './menu/StyledItemTypeIcon';
export * from './menu/StyledButton';
export * from './menu/StyledSeparator';
23 changes: 0 additions & 23 deletions packages/dropdowns/src/views/menu/StyledButton.ts
View file
Open in desktop

This file was deleted.

8 changes: 8 additions & 0 deletions packages/grid/src/elements/pane/components/SplitterButton.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,12 @@ describe('SplitterButton', () => {

expect(getByLabelText('toggle a').querySelector('svg')).toBeDefined();
});

describe('`data-garden-id` attribute', () => {
it('has the correct `data-garden-id`', () => {
const { getByLabelText } = render(<UncontrolledTestSplitter />);

expect(getByLabelText('toggle a')).toHaveAttribute('data-garden-id', 'buttons.icon_button');
});
});
});
7 changes: 1 addition & 6 deletions packages/grid/src/styled/pane/StyledPaneSplitterButton.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@
*/

import styled, { css, ThemeProps, DefaultTheme } from 'styled-components';
import { retrieveComponentStyles, DEFAULT_THEME } from '@zendeskgarden/react-theming';
import { DEFAULT_THEME } from '@zendeskgarden/react-theming';
import { ChevronButton } from '@zendeskgarden/react-buttons';
import { Orientation } from '../../types';

const COMPONENT_ID = 'pane.splitter_button';

interface IStyledSplitterButtonProps {
orientation: Orientation;
isRotated: boolean;
Expand Down Expand Up @@ -52,7 +50,6 @@ const transformStyles = (props: IStyledSplitterButtonProps & ThemeProps<DefaultT
};

export const StyledPaneSplitterButton = styled(ChevronButton).attrs<IStyledSplitterButtonProps>({
'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION,
isBasic: true,
isPill: true,
Expand All @@ -61,8 +58,6 @@ export const StyledPaneSplitterButton = styled(ChevronButton).attrs<IStyledSplit
${sizeStyles};

${transformStyles};

${props => retrieveComponentStyles(COMPONENT_ID, props)};
`;

StyledPaneSplitterButton.defaultProps = {
Expand Down