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.

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

feat(peer-deps): extend styled-components support to v6 #1978

Merged
merged 14 commits into from
Dec 5, 2024

Conversation

ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Nov 15, 2024

Description

This PR paves the way to making Garden compatible with [email protected].

Detail

  • Fix broken specs due to whitespace mismatches
  • Convert props to transient props when necessary
  • Fix stories to prevent invalid DOM attributes
  • Fix TS type errors

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 in dark mode
  • 🤘 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

@coveralls
Copy link

coveralls commented Nov 15, 2024

Coverage Status

coverage: 95.649% (-0.008%) from 95.657%
when pulling 07ecb06 on ze-flo/styled-components-v6
into f345fa1 on main.

package.json Outdated
"stylelint": "16.10.0",
"stylelint-order": "6.0.4",
"stylis": "4.3.4",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing this added dependency used anywhere.

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 migration docs are misleading. 🤔 stylis is a dependency of styled-component. Therefore, this can be removed. Good catch. 💯

packages/breadcrumbs/src/styled/StyledChevronIcon.tsx Outdated Show resolved Hide resolved
packages/chrome/src/elements/header/HeaderItem.tsx Outdated Show resolved Hide resolved
packages/colorpickers/src/styled/ColorSwatch/StyledIcon.ts Outdated Show resolved Hide resolved

export const ThemeContext: React.Context<DefaultTheme>;
export interface ThemeProviderProps<T extends object, U extends object = T> {
children?: React.ReactNode | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
children?: React.ReactNode | undefined;
children?: ReactNode | undefined;

...with import { ReactNode } from 'react';

Comment on lines 87 to 91
${props.theme.rtl ? '-45deg' : '45deg'},
transparent,
${getBackgroundColor},
transparent
);
Copy link
Member

Choose a reason for hiding this comment

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

Please restore indentation, if possible.

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've restored the indentation but extra whitespaces have to be accounted for when using toHaveStyleRule with styled-components@v6. Is that acceptable?

packages/chrome/src/elements/nav/Nav.tsx Outdated Show resolved Hide resolved
packages/theming/src/utils/StyledBaseIcon.ts Outdated Show resolved Hide resolved
@@ -38,7 +39,9 @@ export const Breadcrumb = forwardRef<HTMLElement, HTMLAttributes<HTMLElement>>((
<>
<StyledBreadcrumbItem>{child}</StyledBreadcrumbItem>
<StyledCenteredBreadcrumbItem>
<StyledChevronIcon />
<StyledChevronIcon>
<ChevronRightStrokeIcon />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our SVGO config adds aria-hidden="true" to svgs.

packages/tags/src/elements/Avatar.tsx Outdated Show resolved Hide resolved
Comment on lines 87 to 91
${props.theme.rtl ? '-45deg' : '45deg'},
transparent,
${getBackgroundColor},
transparent
);
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've restored the indentation but extra whitespaces have to be accounted for when using toHaveStyleRule with styled-components@v6. Is that acceptable?

packages/tags/src/elements/Avatar.tsx Outdated Show resolved Hide resolved
@@ -24,7 +24,7 @@ export { useWindow } from './utils/useWindow';
export { useText } from './utils/useText';
export { default as menuStyles } from './utils/menuStyles';
export { focusStyles, SELECTOR_FOCUS_VISIBLE } from './utils/focusStyles';
export { StyledBaseIcon } from './utils/StyledBaseIcon';
export { StyledBaseIcon, type IStyledBaseIconProps } from './utils/StyledBaseIcon';
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with exporting this, but:

  1. I wouldn't expect us to reuse within other packages; in other words, it's for consumers-only
  2. it should be moved to the type export section below

packages/theming/src/utils/StyledBaseIcon.ts Outdated Show resolved Hide resolved
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

fantastic work here 🎉

@ze-flo
Copy link
Contributor Author

ze-flo commented Dec 5, 2024

The work done in #1985 helped me identify props rendered as invalid attributes. 😌

refactor(modal header): use transient props
c4b0b11

fix: prevent invalid DOM attributes
1370c7c

fix(splitter): prevent invalid DOM attrs
c64c2e0

In stories, HTML elements receiving theme colors render invalid color.light and color.dark attributes.
Screenshot 2024-12-05 at 8 49 24 AM

This will be resolved in a future PR.

@ze-flo ze-flo changed the title chore: upgrade to [email protected] feat(peer-deps): extend styled-components support to v6 Dec 5, 2024
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Does the theming package need its styled-components range expanded?

@ze-flo
Copy link
Contributor Author

ze-flo commented Dec 5, 2024

Does the theming package need its styled-components range expanded?

Good call out. Fixed. ✅

@ze-flo ze-flo force-pushed the ze-flo/styled-components-v6 branch from 902f12f to 07ecb06 Compare December 5, 2024 20:49
@ze-flo ze-flo merged commit bc5c1d4 into main Dec 5, 2024
8 checks passed
@ze-flo ze-flo deleted the ze-flo/styled-components-v6 branch December 5, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants