-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RSS: Added Colour support #66419
base: trunk
Are you sure you want to change the base?
RSS: Added Colour support #66419
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @benazeer-ben! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
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.
Great to see the RSS block getting some love. I think this will work well in conjunction with #66411
Here's my ghastly example:
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.
Great work on all these block support PRs @benazeer-ben and thank you for your patience handling all the edge cases 🙇
I've left some comments piggy-backing on Ramon's earlier review. In particular, we might be able to improve the adoption of text-decoration support as detailed in this comment.
Also, with the block being server-side rendered, there's a small issue with the background color needing to be reset on the inner rendering of the block (once server-side rendering skips block support attributes as proposed in your border and spacing support PR).
The Tag Cloud block has a PR adopting color support and the background reset issue came up there.
These style resets are similar to the ones added in your PR adopting border and spacing support for this block.
While reviewing the Tag Cloud PR, I found the cleanest approach to reset the styles to avoid issues with the server-side rendering was the following:
.wp-block-rss .wp-block-rss {
all: inherit; /* This addresses borders, background colors etc. */
margin: 0;
padding: 0;
}
Yes, this is handling in that 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.
This is working pretty well for me, thanks again.
Editor
Kapture.2024-11-14.at.13.18.14.mp4
Frontend
I think I left more questions than answers, sorry.
I also noticed in the editor, block attributes are applied to wrapper and server side rendered component.
I'm not sure this is an issue for color and typography, at least from what I can tell, but I assume other styles like border will have to be handled in a way similar to tag cloud, that is, by modifying the attributes sent to the serverside component.
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.
Appreciate the patience iterating on these PRs @benazeer-ben 👍
I'd like to propose splitting this PR into two. One that adopts color support and another adopting typography support. Any hold-ups to one (looking at you text-decoration
) won't delay the other.
These style resets are similar to the ones added in your PR adopting border and spacing support for this block.
Yes, this is handling in that #66411.
If this PR is dependent on changes in another PR, we need that to be communicated in the PR description and testing instructions.
I know it involves more juggling, but basing this PR off the one it depends on makes things easier for reviewers and testers who lack full context. This way, the branch being tested includes all necessary changes and integrates well with them. Once the other PR is merged, a quick rebase is all that’s needed before merging this one.
+1 |
Co-authored-by: Ramon <[email protected]>
Sure, I will keep this PR only for color support and move typography support from here to a new one. |
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.
Thanks for splitting the PRs up @benazeer-ben
It's a lot easier to test!
Here's what I tested:
✅ RSS titles reflect global styles Link element colors (including hover)
✅ Changes to RSS global styles colors affect all blocks. Links colors overwrite Link element defaults. Background gradients work as expected.
✅ Block-level colors work as expected and take precedence over everything.
✅ Tested the above hierarchy with theme.json (see below)
I think this is good to go if @aaronrobertshaw agrees.
Example theme.json
{
"$schema": "https://schemas.wp.org/trunk/theme.json",
"version": 2,
"settings": {
"appearanceTools": true
},
"styles": {
"elements": {
"link": {
"color": {
"background": "red",
"text": "green"
},
":hover": {
"color": {
"background": "orange",
"text": "teal"
}
},
":active": {
"color": {
"background": "peach",
"text": "cottoncandy"
}
}
}
},
"blocks": {
"core/rss": {
"color": {
"background": "yellow",
"text": "pink"
},
"elements": {
"link": {
"color": {
"background": "aquamarine",
"text": "white"
},
":hover": {
"color": {
"background": "purple",
"text": "black"
}
}
}
}
}
}
}
}
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've given this a quick smoke test as well after splitting the PR. It's working nicely for me.
What?
Add color support to the RSS block.
Fixes #43245
Why?
RSS block is missing color support.
How?
Adds the color support via block.json
Testing Instructions
NOTE:
This PR is dependent on some style changes in another PR
Screenshots or screencast
Backend:
Frontend: