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

fix(theming): allow theming of typescript consumers #984

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

austingreendev
Copy link
Contributor

@austingreendev austingreendev commented Jan 14, 2021

⚠️ This PR includes a rename to an existing export. Normally this would be a breaking change, but I'm categorizing this as a fix as the original export wasn't providing any information. ⚠️

Description

This PR modifies how we provide and consume our GardenTheme interface within react-theming. These changes will make it possible for other codebases to more easily consume our theme within TypeScript codebases.

Detail

styled-components requires consumers to define their own DefaultTheme within their codebase. We've accomplished this in our react-components and website repositories.

Up to this point we have been exporting the DefaultTheme object from styled-components within the react-theming package. Due to the ponyfill approach above this has actually been exporting an empty ({}) interface this whole time 🤦

Approach

The new approach is to define a separate IGardenTheme interface that defines the expected structure of the Garden theme. This new interface is only used in the ThemeProvider component. All other usages and utilities still consume the DefaultTheme from styled-components to allow consumers the flexibility to customize the theme structure.

For consuming codebases this would be the format to type their theme:

import 'styled-components';
import { IGardenTheme } from '@zendeskgarden/react-theming';

declare module 'styled-components' {
  export interface DefaultTheme extends IGardenTheme {}
}

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 🌐 demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • ♿ analyzed via axe and evaluated using VoiceOver
  • 💂‍♂️ includes new unit tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

@@ -112,7 +209,7 @@ const lineHeights = {
const palette = { ...PALETTE };

/* Exclude product palette from the theme */
delete palette.product;
delete (palette as any).product;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think another way around this without using any is...

const { ...palette, product } = PALETTE;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since PALETTE is inferring its type this can cause a bunch of Index signature is missing in type errors. The original ... operator is what keeps this working.

We probably shouldn't be inferring those types anyway, but that is probably a larger scope change than we should do here.

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.

nice.

@zendesk-garden zendesk-garden temporarily deployed to staging January 14, 2021 18:21 Inactive
@austingreendev austingreendev merged commit 4c75d91 into main Jan 14, 2021
@austingreendev austingreendev deleted the agreen/styled-theming branch January 14, 2021 22:04
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.

None yet

5 participants