Skip to content

Conversation

@ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Aug 23, 2024

Description

This initiates the process of removing defaultProps to avoid deprecation warnings, focusing first on theme. Since v9 depends entirely on ThemeProvider and its theme for dark mode, rendering a component without wrapping it in ThemeProvider should result in failure.

Detail

  • Removes eslint-plugin-garden-local
  • ThemeProvider: sets the theme prop's default to DEFAULT_THEME
  • Removes theme defaultProp in all components
  • Removes unnecessary test

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

@ze-flo ze-flo marked this pull request as ready for review August 23, 2024 21:56
@ze-flo ze-flo requested a review from a team as a code owner August 23, 2024 21:56
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.

Unfortunately, I don't think this is the desired change. Garden element components need to continue to render even when not wrapped by a ThemeProvider. In other words, this change needs to occur in a non-breaking fashion. Most likely this is via argument defaults on element components which are in-turn passed down to view components. But some exploration/discussion may be necessary. Furthermore, the changes must be validated via npx garden cmd-docgen --pretty <path> to ensure that every affected component retains the expected prop documentation.

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

+1 JZ's comment. I was under the impression we wanted to keep theme as a default prop but explore how that can be achieved with styled components first, as the changeset is pretty big (as this PR is evident).

Also I remember we talked about keeping the initial change isolated on ThemeProvider as its the biggest culprit of console warnings in development. 😅

@ze-flo
Copy link
Contributor Author

ze-flo commented Aug 30, 2024

Here's a table comparing popular React component libraries written with styled-components or emotion.js. All support theming and light / dark mode. The table highlights whether each library can render components without first nesting them inside a ThemeProvider.

Library Works without ThemeProvider Notes Docs Sandbox
@mui/material DefaultProps injected via React's context API with defaultPropsProvider https://mui.com/material-ui/getting-started/usage/ https://codesandbox.io/p/sandbox/jgtp7x
@mantine/core https://mantine.dev/guides/vite/#setup
@primer/react https://primer.style/guides/react
@chakra-ui/react https://v2.chakra-ui.com/getting-started
@adobe/react-spectrum Components requiring theme render null if its missing. Others render unstyled. https://react-spectrum.adobe.com/react-spectrum/getting-started.html#setting-up-your-app https://codesandbox.io/p/sandbox/admiring-wood-fqqj8h

While @mui/material has a built-in mechanism to handle the absence of a ThemeProvider, most tested libraries strictly require it to be set up for proper rendering and styling. The behavior varies across different libraries, with some failing to render components and others rendering unstyled components without a theme.

@jzempel
Copy link
Member

jzempel commented Sep 4, 2024

@ze-flo I think this is pretty compelling evidence that you're on the right path with this PR. Can we get the migration.md updated with the breaking change to theming and either add a corresponding ADR or have a plan to add it immediately after?

@ze-flo
Copy link
Contributor Author

ze-flo commented Sep 5, 2024

@ze-flo I think this is pretty compelling evidence that you're on the right path with this PR. Can we get the migration.md updated with the breaking change to theming and either add a corresponding ADR or have a plan to add it immediately after?

I updated the migration docs and created a ticket to add the corresponding ADR in a follow-up PR.

Co-authored-by: Jonathan Zempel <[email protected]>
@ze-flo ze-flo merged commit 664ad79 into main Sep 5, 2024
@ze-flo ze-flo deleted the ze-flo/remove-default-theme-prop branch September 5, 2024 21:40
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.

4 participants