Font Appearance Control: Refactor font appearance fallbacks #63215
+554
−51
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
mikachan
added
[Type] Bug
|
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. |
|
Size Change: -2.53 kB (-0.14%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
talldan
approved these changes
Jul 8, 2024
There was a problem hiding this comment.
The changes look good, the code is much neater, while also fixing that problem. Thanks for following up so quickly.
The one possible issue I'm seeing is that when adding a block programmatically using the snippet, the appearance dropdown still shows 'default'.
I guess that leads to a philosophical question of whether an absence of a fontStyle means some kind of default, which is tricky as the theme might have some custom css setting the font style to something else entirely. 🤔
It might be ok to leave it as it is, I'm not too familiar with the feature or the code, so it might be good to gather opinions from those more in the know.
| // Reset font appearance if the font family does not have the selected font style | ||
| if ( ! hasFontStyle ) { | ||
| } else { | ||
| // Reset font appearance if there are no available styles or weights | ||
| resetFontAppearance(); | ||
| } | ||
| }, [ fontFamily ] ); |
There was a problem hiding this comment.
There are some missing dependencies, which results in a linter warning.
I think it'd be good to make sure that's fixed or the rule is disabled for this line (with a comment mentioning why).
To fix it, it looks like setFontAppearance and resetFontAppearance would have to be wrapped in useCallback.
There was a problem hiding this comment.
I tried adding all of the dependencies but it looks like this causes an infinite loop, and I think it's OK to rely on only the fontFamily changing here as this is the main prop we want to watch in this case.
I've disabled the rule for this line with a comment explaining why: 3796305
I left the useCallback functions in for setFontAppearance and resetFontAppearance as they seem to work the same either way, and I'm guessing this might have some small performance benefits.
packages/block-editor/src/components/global-styles/typography-panel.js
Outdated
Show resolved
Hide resolved
|
Thanks for the quick review, @talldan!
I think this should only happen if there are no available styles/weights from the active font family for the settings passed in this snippet. This should be unlikely if a system font is active, as this should have all of the available styles and weights. Do you still see this if a system font is active before you run this snippet? Here's an example of running the snippet with just a weight setting of 300: Screen.Recording.2024-07-08.at.11.41.24.mov |
|
I've added some e2e tests to help ensure the Appearance dropdown is updated correctly, based on the snippet that adds a new block with appearance settings: 7e027b4. I've also split the "finding the nearest font style" logic out into its own function: 72199cc. There should be no difference in functionality, I just thought this was better as it's easier to read and more closely matches the font weight logic. |
There was a problem hiding this comment.
This seems to be functioning as expected for all of the test cases discussed in #61915.
And I can see how programmatic block insertion now has the specified weight and style appearance.
Screen.Recording.2024-07-08.at.16.36.59.mov
| * | ||
| * @param {Array} availableFontStyles Array of available font weights. | ||
| * @param {string} newFontStyleValue New font style value. | ||
| * @return {string} Nearest font style. |
There was a problem hiding this comment.
It looks like this function could also return undefined if none of the conditionals are triggered, is that right? It so, let's update the return type here.
There was a problem hiding this comment.
Oh good spot! My intention was that it would default to an empty string if none of the conditionals were triggered. I've updated the function here: b407be1, I think this should cover all branches.
mikachan
added
the
Backport to Gutenberg RC
|
Thanks for the reviews @talldan and @creativecoder! I'd appreciate one more approval before bringing this in as there have been a couple of changes since the last approval. I think this is ready now though, thank you for the help 🙇 |
|
I reported an issue in #63479 where inserting a pattern would create multiple undo histories. This PR seems to solve that issue as well. |
There was a problem hiding this comment.
My results testing this PR today:
I manually changed a theme.json fontStyle for one of the fonts to oblique for testing.
- ✅ Switch from font with Bold Italic to Black Italic and back
- Both trunk and PR: Appearance setting switches to nearest font weight
- ✅ Switch from font with Oblique Regular or Bold settings to one without Oblique
- trunk: Appearance setting switches to Default
- PR: Appearance setting switches to nearest font weight with italic
- ❓Insert block from console with unavailable font weight and style
- trunk: Appearance setting is Default, font does not display the specified appearance
- PR: Appearance setting is Default, but font does display the nearest weight and style
Number 3 is different than my earlier test, so seems like a regression?
Another question I have: should we not add the faux italic appearance if the font already has an oblique style (it seems redundant)? But we could handle this in a follow-up, if needed.
Thanks @creativecoder. Yes, I think it is a regression. I think the logic for checking if the font appearance settings have changed should sit in
Good point, this sounds like a good follow-up! |
talldan
approved these changes
Jul 15, 2024
There was a problem hiding this comment.
This is still working well for me with the latest changes, though I haven't tested as many aspects as @creativecoder, so it might be good to also check for Grant's approval.
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?
Refactors the logic around the font weight and style fallbacks applied when a new font family is selected. Follow-up to #61915.
Why?
Addresses this comment: #61915 (comment).
How?
Adds a new
findNearestStyleAndWeightfunction that finds the nearest available weight and style to apply when the font family is changed, regardless of whether the font family is selected using global styles or dynamically via a new block. Also, as the logic now sits in a function, it is easier to add automated tests.Testing Instructions
Ensure there are a variety of font faces installed in multiple font families, including different font weights, styles and at least one system font.
Try running the following in the console:
The nearest available font weight and style should be automatically applied.
Screenshots or screencast