-
Notifications
You must be signed in to change notification settings - Fork 0
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
[DNM] Use multiple swatches in Svingle #629
base: main
Are you sure you want to change the base?
Conversation
This converts the Svingle theme to allow the use of multiple swatches. The swatch is selected by name and typos or non-existent color swatches will throw compiler errors. This is also not backwards breaking. Ticket: https://phabricator.wikimedia.org/T380274
Here is the documentation for the new sass features that were introduced to the project in this PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm amazed at what's possible with SCSS and open to the idea of introducing "color subthemes".
The only change request I have is for adding some documentation to the docs/Swatches.md
document. We need to document
- How subthemes work internally (using SCSS Placeholder selectors), explaining that weird
%selector
syntax - How to create a new color subtheme with swatches (adding a folder with the swatch files, adding the SCSS placeholders to the
skin_default
andskin_vector_2022
)
I thought long and hard one the benefits and drawbacks of "adding 'color subthemes' through the swatch mechanism" vs "forking the theme". Here are my thoughts:
Forking the theme
- Would give each theme a specific "identity" (i.e. name), making the mental model easier for the developers (and tools like
show_theme_usage.sh
and future static analysis tools), because they'd only have to look at the path names instead of the path name and the special$swatch
variable to determine the visual appearance.
Color swatches
- Violate the "Open-Closed Principle" that "extending" the swatches with a new feature means adding new parameters to all the swatch files, with the error potential of misplacing the positional parameters. This means that we will be maintaining color schemes for the rest of the campaign, even if they are (re)used. A compromise here would be to put the swatches for the the first A/B test into the banner directory and if VAR wins, convert it into a color subtheme.
- Makes the
skin_default
andskin_vector_2022
files longer and harder to navigate. You also lose the ability to compare the SCSS placeholder selectors (to detect if you misplaced or forgot a positional parameter). This problem would also be mitigated a bit by the "provisional swatch in the banner folder, if it wins, put it in the theme folder" approach. - Will make it easy to copy color schemes between themes.
- Make the maintenance of all non-color related CSS styles easy and DRY for all themes shared between channels (we use
Treedip
for desktop EN, iPad and WPDE desktop andMikings
for mobile DE and WPDE). With a fork, we'd have to copy fixes for display bugs and CSS files (and@use
and@if
statements for them) between the themes (and make them inconsistent and evolve into different directions). For me, this is the "killer usecase" that makes the swatch approach superior, even with all the drawbacks outlined.
} | ||
|
||
.wmde-banner { | ||
@extend %#{$swatch}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this is some serious level of indirection here 😺
This converts the Svingle theme to allow the use
of multiple swatches. The swatch is selected by name
and typos or non-existent color swatches will throw
compiler errors.
This is also not backwards breaking.
Ticket: https://phabricator.wikimedia.org/T380274