-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fixes FSE Navigation sub-menu item styling regression #31754
Conversation
Size Change: +9 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
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.
Working for me:
menu.mp4
Thanks for the PR. I think this is a nice fix, especially because it's so small. But before you merge, I'm not sure it actually addresses the root cause. Why did it regress in the first place? Also, the black color is still there. Should it be? Here's before: Note how there's a black color explicitly defined on the inner span. Here's after: Now the black color is overridden by the newly added inherit rule. But why did it work before, what broke it, and where does the black come from? |
This fix is probably fine to land if it needs to go into a point release. But we should probably figure out how it regressed, even if it needs a bisect. It feels weird to me that there's a black color explicitly defined on that inner component. |
Looks like it's still experimental and has only recently been introduced. If I do a quick search, the few locations it is being used have all defined a color property... Not sure what made it work in the past though. |
Alright I did a little digging, and #30661 is the last commit where the text had the right color: The next one in the list is the one that regressed the color, that's #31291. Looking at that component, it has this value:
from https://github.com/WordPress/gutenberg/pull/31291/files#diff-e4b73be6ec3057a00d7a0f3252abb299f13b34b45d7510ce75187887ac5cd1b4L12 — that's an unchanged value. But comparing the before and after, it looks like this Text component wasn't used inside the Button component in the previous iteration. So I guess there are two questions to answer here: should the Text component be used inside an "ItemUI" component? And also, should a Text component come pre-set with a black color, or should that color just always be inherited? |
Okay, let's do as you suggested. Use this change for the patch release, as we can be sure this does not impact anything else (or does it impact RN?). We can then get this looked at in a follow-up issue.
Not sure, I just assumed this was part of some kind of effort to convert all |
Fine to land this as a patch, yes, but I'd love to hear what's a good longer term fix that might also remove the bandaid if possible. CC: @gziolo @sarayourfriend in case you have answers to #31754 (comment). |
I do agree that it seems weird for me to enforce a color by default in the Text component. |
Id think this makes sense as a patch fix. However, also agree the |
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 for taking care of this! ✨
I'm merging this for now as a fix for the patch release, any volunteer for the follow-up for the Text component? |
Attempts to fix the regression of the FSE Navigation sub-menu item styling as reported in #31574.
Closes #31574.
Screenshot:
Right now, it uses
inherit
as color. Not sure about the text color before this issue, but it seems to be the right color now without adding more complexity, but that might not be the correct approach.My original suggestion was to move back to a
span
element, but I'm not sure on plans for thatText
component, so this might be a better solution.