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

refactor!: Remove 'theme' props from components #2194

Merged
merged 7 commits into from
May 21, 2021

Conversation

driskull
Copy link
Member

@driskull driskull commented May 21, 2021

Related Issue: #2189

Summary

This PR is the first step of removing the use of the theme attribute selector.

  • Removes all 'theme' properties from components
  • Removes all use of the theme properties in tests
  • Adds css class selectors for light/dark themes

The next steps would be to

  • Replace theme attribute selector with classes
  • Update stories to replace 'theme' properties with classes.
  • Update test apps to replace 'theme' properties with classes.
  • Document the theme classes somehow

Please review

@driskull driskull changed the title DRAFT: remove theme props from components and tests refactor!: Remove 'theme' props from components May 21, 2021
@driskull driskull self-assigned this May 21, 2021
@driskull driskull added the 1 - assigned Issues that are assigned to a sprint and a team member. label May 21, 2021
@driskull driskull marked this pull request as ready for review May 21, 2021 19:31
@driskull driskull requested a review from a team as a code owner May 21, 2021 19:31
src/utils/dom.ts Outdated Show resolved Hide resolved
@paulcpederson
Copy link
Member

nice! tons of work, thanks @driskull . I don't think we should be using getComputedStyle unless we absolutely have to, but other than that looks good to me

@driskull driskull requested a review from paulcpederson May 21, 2021 22:08
@driskull
Copy link
Member Author

For now, closest will look for either the class or attribute. Once we remove all uses of the attribute we can update the utility to remove that part.

src/utils/dom.ts Outdated Show resolved Hide resolved
@paulcpederson
Copy link
Member

I think you may be able to remove the theme name vars altogether?

LGTM, though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants