Doist / reactist

Open source React components made with ❤️ by Doist
http://doist.github.io/reactist
MIT License
250 stars 22 forks source link

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

Closed frankieyan closed 1 year ago

frankieyan commented 1 year ago

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

Versioning

Minor

frankieyan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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!