Fix font variants count color contrast ratio and l10n. #58117
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afercia
added
[Type] Bug
|
Size Change: +31 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Jan 23, 2024
There was a problem hiding this comment.
Both changes make sense to me.
I found another place where a similar fix might be needed in the Google Fonts tab of the modal dialog. How about addressing this part with this PR?
By the way, this text has a hardcoded #6e6e6e color, so I think we can replace it with $gray-700;.
|
@t-hamano thanks for your review. From an accessibility perspective, that is okay. Rather, the problem is that it's not a color included in the default color palette. I'd argue that also the border width and other values should preferably use variables, where appplicable and desirable. Overall, I think all the styesheets of the Fonts Library UI should be reviewed as they appear to not follow the Gutenberg best practices. However, that's a broader issue that it's outside of the scope of this PR. @WordPress/gutenberg-design @matiasbenedetto is there any reason why these stylesheets use colors that are not from the default color palette? Hardcoding 'magic' values across the codebase doesn't seem to be the best way to maintain design consistency to me. |
I see, in that case, I think there is no need to change the color in this PR.
On this point, I completely agree. I'm considering working on replacing all hard-coded color codes and margin/padding values in the stylesheet with CSS variables. As @afercia has reported various issues, accessibility also needs to be re-tested as a whole. |
I completely agree, at the point I'm not sure I would consider the current stylesheets of the fonts librady / modal as 'ready to be merged' in core. I'n the meantime, since the first review was okay, can I have a quick approval of this PR please? 🙂 |
t-hamano
approved these changes
Jan 24, 2024
There was a problem hiding this comment.
LGTM 👍There are probably two follow-up issues that can be tackled immediately:
- Update variation text in the Google Fonts tab of modal dialogs to align with localization best practices
- Replace all hard-coded color codes and margin/padding values in the stylesheet with CSS variables (I'll be happy to do it)
Nope, thanks for improving it 👍 |
|
follow-ups:
@t-hamano thanks, I can create the issue for the l10n best practices. |
|
@getdave @youknowriad as Editor Tech Leads for the 6.5 release and @WordPress/gutenberg-design I'd appreciate your feedback on whether using colors from the color palette is a requirement to consider any code 'ready to be merged'. When you have a chance ❤️ Thanks. |
Sure! I'll be working on it soon so we can ship it to WP6.5. |
|
Obviously, it's recommended to use the variables for CSS and avoid inline styles like. There are a lot of guidelines for developers working on Gutenberg and I don't believe it's the role of the release leads to go into each PR and check that the guidelines have been followed. For me mistakes happen and it's our collective responsibility to ensure they are followed. We should always assume good intent as well, mistakes happen. |
|
@youknowriad sure, my intent isn't blaming anyone. I'm more interested in clarifying the process and make sure we all are on the same plan. But I do realize GitHub isn't the best place to discuss this. |
|
Firstly, thank you for this PR. It's a good example of clean up if/when things do slip through the net. Thank you for following up. I would say that as part of a review process, reviewers should look at whether the PR is using standardised variables for CSS values wherever possible. As you will appreciate, it cannot be a hard rule as there will inevitably be exceptions. I agree with Riad that this guidance applies outside of any particular release cycle. |
This has been fixed in #58237. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #58110
What?
_n()function doesn't follow the WordPress localization best practices.Why?
How?
$gray-700color, which is the lightest gray that can be used on a white background.sprintfand a placeholder instead of concatenation.Testing Instructions
#757575.Screenshot before and after:
Testing Instructions for Keyboard
Screenshots or screencast