-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add a prominent style variation for the language suggestion. #607
Conversation
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.
A few minor notes, but overall tested out well. I was able to use the prominent class as-is, and also combined with custom colors. I also tested the default style, just to make sure there were no regressions.
} | ||
|
||
.wp-block-wporg-language-suggest.is-style-prominent > * { | ||
--wp--custom--wporg-language-suggest--spacing: var(--wp--preset--spacing--20); |
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.
Should this also have a fallback value, since the others do?
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.
Could this also be set in .wp-block-wporg-language-suggest.is-style-prominent {}
? I think that should cascade down correctly, right?
--wp--custom--wporg-language-suggest--background: var(--wp--preset--color--blueberry-4, #eff2ff); | ||
--wp--custom--wporg-language-suggest--color--text: var(--wp--preset--color--charcoal-1, #1e1e1e); | ||
--wp--custom--wporg-language-suggest--font-size: var(--wp--preset--font-size--extra-small, 12px); | ||
--wp--custom--wporg-language-suggest--spacing: var(--wp--preset--spacing--10, 10px); |
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.
These are named like they can be set in theme.json, but they'll be overridden here if they are. You could put them in a :root
block, so any theme.json settings would override it, but that would still be overridden by the prominent style (if you did, I'd also suggest renaming --wp--custom--wporg-language-suggest--background
to --wp--custom--wporg-language-suggest--color--background
so the object syntax would be correct).
Not a strong suggestion, I'm not sure it would be used if this was possible, but I tried it out since they're named matching the syntax.
"settings": {
"custom": {
"wporg/language-suggest": {
"background": "var(--wp--preset--color--pomegrade-1)"
}
}
},
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.
Got it. 👍
I've made some updates to look first for theme.json generated variables and then fallback in 92eb3f7. Thoughts?
In terms of following object syntax, because I'm making use of the "custom" property, I have gone with the shortest name possible. If that breaks convention or isn't developer friendly, I don't mind updating. The css declarations were just getting long enough :).
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.
Example usage in theme.json:
"custom": {
"wporg-language-suggest": {
"background": "red"
}
}
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.
The global styles (theme.json) are loaded after the block styles, so you can just set up these variables in this stylesheet in :root
and they'll be overwritten if the theme defines them:
:root {
--wp--custom--wporg-language-suggest--color--background: var(--wp--preset--color--blueberry-4, #eff2ff);
--wp--custom--wporg-language-suggest--color--text: var(--wp--preset--color--charcoal-1, #1e1e1e);
--wp--custom--wporg-language-suggest--typography--font-size: var(--wp--preset--font-size--extra-small, 12px);
--wp--custom--wporg-language-suggest--spacing: var(--wp--preset--spacing--10, 10px);
}
This ^ also uses the property names based on how the styles are broken down in settings - even though this is custom
and does not have any standards, I would lean to mirroring the syntax from styles
. So background
and text
are properties of color
, and font-size is under typography
. The exception is that spacing
should be margin
or padding
objects, but we only have the one spacing value so leaving it as a string works.
"wporg/language-suggest": {
"color": {
"background": "var(--wp--preset--color--white)",
"text": "var(--wp--preset--color--charcoal-1)"
},
"typography": {
"fontSize": "18px"
},
"spacing": "24px"
}
On the other hand, I think you could stick with --wporg-language-suggest--background
-style names, and drop the theme.json
connection entirely. As long as it works with local modifications we'll probably be fine, and it does.
<!-- wp:wporg/language-suggest {"align":"full","style":{"elements":{"link":{"color":{"text":"var:preset|color|white"}}}},"backgroundColor":"purple-1","textColor":"white","className":"is-style-prominent"} -->
<div class="wp-block-wporg-language-suggest alignfull is-style-prominent has-white-color has-purple-1-background-color has-text-color has-background has-link-color"></div>
<!-- /wp:wporg/language-suggest -->
![Screenshot 2024-05-09 at 11 41 56 AM](https://private-user-images.githubusercontent.com/541093/329301974-74c73aae-2058-45c2-99a6-49099ceffbdb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzMTY1NjYsIm5iZiI6MTczOTMxNjI2NiwicGF0aCI6Ii81NDEwOTMvMzI5MzAxOTc0LTc0YzczYWFlLTIwNTgtNDVjMi05OWE2LTQ5MDk5Y2VmZmJkYi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjExJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxMVQyMzI0MjZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jZDYxMWMzNjhjMzVmMjQxMzMyNDZhMDNjYmM3ZjQzMTE2ZWRlYjVlYTFkYmEzZDNlYWI3NDk5MjgzNzJlNjhmJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.15WYE7g-oKex0RNeoxoDbtUc1_VoWFWSbhOhfCbLio0)
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 agree. I'll keep it local and merge. Thanks for the review.
Relating to WordPress/wordpress.org#301, this PR adds a new style variation to the language suggest block. This will allow us to set it in a theme template.
For the case of the plugin directory where it can be conditional based on the page, I assume we can just filter it as follows:
Screenshots
is-prominent