Fix item font family item height in the sidebar #63125
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
|
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. |
matiasbenedetto
added
[Type] Bug
|
Size Change: +9 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
mikachan
reviewed
Jul 4, 2024
|
Flaky tests detected in a60266a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10093000158
|
|
This PR makes the itemgroup items closer to 40px than 32, so that's good:
A few things, but they may be more on the component side, so CC @tyxla:
But overall, the intent of this PR is right, I'm just not sure this is the right fix on the code level. Seems like this should be fixed in the component itself? |
tyxla
approved these changes
Jul 4, 2024
|
If we need to always have the same item height, we might have to play with the padding math right there: If we're aiming at a particular item height, it might actually be more straightforward to not rely on padding at all, but rather to apply item height directly and center the contents vertically. cc @WordPress/gutenberg-components |
Agreed!
It looks like the As we're looking at stabilizing
|
|
If the |
|
Can you resolve the conflicts and merge this PR? In trunk, fonts are grouped by source.
By the way, there is an issue proposing to revert this grouping: #63505 |
4 tasks
# Conflicts: # packages/edit-site/src/components/global-styles/font-families.js
Done! I'll enable auto-merge. |
mikachan
added
[Feature] Font Library
and removed
Needs Design Feedback
|
Sorry, I missed it, but don't we need to change the height of the custom fonts? Here it is: |
Thanks @t-hamano! I missed it too, I think I managed to push the change just before the auto-merge happened 👀 |
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.
What?
Fix item font family item height in the sidebar
Why?
To match the other components height
How?
Setting the large prop in the component
Screenshots or screencast