Skip to content

feat: Expose CSS variables for theming the SwitchField component#765

Merged
frankieyan merged 3 commits intomainfrom
frankie/switchfield-color-variables
Apr 26, 2023
Merged

feat: Expose CSS variables for theming the SwitchField component#765
frankieyan merged 3 commits intomainfrom
frankie/switchfield-color-variables

Conversation

@frankieyan
Copy link
Copy Markdown
Member

@frankieyan frankieyan commented Apr 25, 2023

Short description

Instead of relying on shared custom properties like --reactist-bg-default to theme the SwitchField, this explicitly lists variables we can use:

image

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)
  • Reviewed and approved Chromatic visual regression tests in CI

Versioning

Minor

@frankieyan frankieyan self-assigned this Apr 25, 2023
@frankieyan frankieyan force-pushed the frankie/switchfield-color-variables branch from bdd4ad2 to 225ae36 Compare April 25, 2023 19:51
@frankieyan frankieyan requested review from a team and gnapse and removed request for a team April 25, 2023 19:52
@frankieyan frankieyan force-pushed the frankie/switchfield-color-variables branch from 225ae36 to ba080ba Compare April 25, 2023 20:42
Copy link
Copy Markdown
Contributor

@gnapse gnapse left a comment

Choose a reason for hiding this comment

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

Looks like a great direction to take.

One thing/question that comes to mind: have you intentionally thought about the naming convention of CSS variables? In particular, I'm worried about the fact that we either name them after a CSS property (e.g. --reactist-bg-…) or after a component (--reactist-switch-…).

What happens if the first word that comes after --reactist-… ever becomes ambiguous? Not that I see any possibility of that happening soon or ever, but still. I wanted to know what you think about this.

Another question I have: how are you going to deal with releasing this? This is technically a breaking change, is it? Especially if you're planning to create more breaking changes in the short term, do you plan to accumulate them? If so, will you accumulate them on main? Perhaps a next branch would be best. Unless you plan to release them as you go.

@frankieyan
Copy link
Copy Markdown
Member Author

Thanks @gnapse, great questions!

how are you going to deal with releasing this? This is technically a breaking change, is it?

I'd say this is technically not a breaking change, because we're setting the variables we had previously used as default values, ie. --reactist-switch-background defaults to --reactist-framework-fill-summit, and --reactist-switch-toggle defaults to --reactist-bg-default. If we'd previously overridden --reactist-bg-default to set the toggle's colour (which we did in Todoist), we aren't forced to make any changes and it would keep on working as is., but I will take care of using the new variables anyway when bringing this into Todoist.

I don't have anymore changes planned in the short term 👍

What happens if the first word that comes after --reactist-… ever becomes ambiguous? Not that I see any possibility of that happening soon or ever, but still. I wanted to know what you think about this.

I had not thought about this, but when I discussed with Paul (ref), we want to bring the product library variables here into Reactist as a mid-term goal, so variables like --reactist-bg should be retired with that change, leaving us with just the component-specific variables. Our reliance on the component-specific variables should also decrease once we are aligned with the product library.

@gnapse
Copy link
Copy Markdown
Contributor

gnapse commented Apr 25, 2023

I'd say this is technically not a breaking change, because we're setting the variables we had previously used as default values

Ah yes. Good point. Indeed, under that consideration it is really not a breaking change.

we want to bring the product library variables here into Reactist as a mid-term goal, so variables like --reactist-bg-* should be retired with that change

Well, in a way it will have to stay. The --reactist-bg-* variables correspond to the Box's background prop.

@frankieyan
Copy link
Copy Markdown
Member Author

Well, in a way it will have to stay. The --reactist-bg-* variables correspond to the Box's background prop.

Good point, we may be able to map them to the product library's background colours and see if that fulfills all of our use cases, but something to look into later in more detail!

@frankieyan frankieyan enabled auto-merge (squash) April 26, 2023 19:16
@frankieyan frankieyan merged commit 2ecbb50 into main Apr 26, 2023
@frankieyan frankieyan deleted the frankie/switchfield-color-variables branch April 26, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants