-
Notifications
You must be signed in to change notification settings - Fork 359
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
Block Canvas: Update background and foreground color slugs to base and contrast #6892
Conversation
…he theme development guideline.
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.
Thank you @iamtakashi for bringing the discussion forward. I left one minor comment, and have one more general thought:
Base and contrast feel like they suffer from the same problems as background and foreground, but are just more abstract names. It made me wonder, what if we didn't have background and foreground, or base and contrast at all?
I opened a PR to try this out with Muscat, because background / base and foreground / contrast values don't really make sense for this theme: https://github.com/Automattic/themes/pull/6894/files. What do we lose with this approach?
Regarding content portability under this scenario, maybe it could be solved if themes had a way to reference element level theme.json values like styles.color.text
and styles.color.background
in the block markup (patterns and templates). Does this exist?
block-canvas/theme.json
Outdated
}, | ||
{ | ||
"color": "#ffffff", | ||
"name": "Base", |
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.
Do you think the order matters here? In the UI, the order here affects the order they are displayed in the picker:
Looks like TT3 puts base and contrast first: https://github.com/WordPress/twentytwentythree/blob/11440a5b38bae6882fc2c00b703d4bb95420a0f5/theme.json#LL32
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.
Good point. Alphabetical order makes sense to me. I'll make an edit.
Sorry for the delay.
So, the author use whatever makes sense for it? Wouldn't it make content portability worse until core has a solution like you mentioned? The motivation for me was to follow the guideline and escape from the confusing results with those slugs and |
Just dropping a link to a relevant issue created during the development of TT3: WordPress/gutenberg#43627. I'm not sure if there's been any more recent discussion around this. |
I think we should at least match TT3, as it gets us away from the use of background and foreground. Going to merge this one. |
The
background
andforeground
colour slugs have been issues for many, but they are still around because of the content portability concerns. Yes, we'll have some rough periods in that regard after this change, but I think the sooner, the better to rip the bandaid off :)The people from the community also shared their opinions that favour the
base
andcontrast
slugs during TT3 development.@jffng let me know that these new slugs have been suggested in the theme development guideline here now, and it's time to change.
We're going to continue using Block Canvas as the starting point for many themes to come, and this will be a good step forward to standardisation in colour slugs.
(Sorry, the PR included the changes to indentation—my Sublime does that automatically on save. We should use tabs instead of spaces to follow the coding standards.)