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(theming): add componentStyles utility #1986

Merged
merged 14 commits into from
Dec 12, 2024
Merged

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented Dec 6, 2024

Description

This is an improved version of retrieveComponentStyles, with:

  • improved typing (consumer doesn't need to fuss with data- attribute exceptions in styled components)
  • the ability to specify a custom ID attribute (defaults to data-garden-id)
  • defaults support no-parameter implementations
  • naming aligns better with other Garden view component conventions (sizeStyles & colorStyles)

Detail

All instances of the @deprecated retrieveComponentStyles will be replaced in a follow-on PR.

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

@jzempel jzempel requested a review from a team as a code owner December 6, 2024 15:21
*
* @returns component CSS styles
*/
export const componentStyles = (props: ThemeProps<DefaultTheme> & { componentId?: string }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

ThemeProps was dropped in styled-component v6. To reduce the refactoring scope within our internal styled components, I added back ThemeProps to the styled-components namespace. However, ThemeProps won't be available for users on styled-components v6.

Here's the built output for componentStyles.d.ts showing the missing type with v6.

Screenshot 2024-12-06 at 11 00 19 AM

It's probably best to explicitly define the theme prop.

Suggested change
export const componentStyles = (props: ThemeProps<DefaultTheme> & { componentId?: string }) => {
export const componentStyles = (props: { componentId?: string, theme: DefaultTheme }) => {

@@ -7,7 +7,7 @@

import { ThemeProps, DefaultTheme } from 'styled-components';

/** @component */
/** @deprecated Use `componentStyles` instead. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is /** @component */ needed for the documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

nah, just leftover from ancient Styleguidist notation

@@ -7,7 +7,7 @@

import { ThemeProps, DefaultTheme } from 'styled-components';

/** @component */
/** @deprecated Use `componentStyles` instead. */
export default function retrieveComponentStyles(
componentId: string,
props: Partial<ThemeProps<Partial<DefaultTheme>>>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an oversight from #1978. 😞

Suggested change
props: Partial<ThemeProps<Partial<DefaultTheme>>>
props: { theme?: <Partial<DefaultTheme> }

*/
export const componentStyles = (props: ThemeProps<DefaultTheme> & { componentId?: string }) => {
let retVal: string | undefined;
const components = props.theme?.components;
Copy link
Contributor

Choose a reason for hiding this comment

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

ThemeProps (or my suggestion ☝️) defines theme as required. If this is what we want going forward:

Suggested change
const components = props.theme?.components;
const components = props.theme.components;

@ze-flo
Copy link
Contributor

ze-flo commented Dec 12, 2024

Once last thing I forgot to mention. 😞 Can we update the template for StyledComponents?

@jzempel
Copy link
Member Author

jzempel commented Dec 12, 2024

Once last thing I forgot to mention. 😞 Can we update the template for StyledComponents?

I'll get that in the follow-on PR mentioned in the description 😉

Copy link
Contributor

@ze-flo ze-flo left a comment

Choose a reason for hiding this comment

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

Nice! 💯

@jzempel jzempel merged commit a868259 into main Dec 12, 2024
8 checks passed
@jzempel jzempel deleted the jzempel/component-styles branch December 12, 2024 18:05
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.

2 participants