-
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
Font Appearance Control: Refactor font appearance fallbacks #63215
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. |
Size Change: -2.53 kB (-0.14%) Total Size: 1.75 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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 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.
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.
Does this work better after the last commit - 8288dd4?
Yes, all of my test cases are now working 😄.
Thanks so much for all the reviews and testing! I'll bring this in 🎉 |
* Refactor font appearance fallbacks * Create new findNearestStyleAndWeight function * Add tests for findNearestStyleAndWeight * Refactor findNearestStyleAndWeight * Add a test for normal/100 * Add comments * Tidy up comments * Fix test description * Update test descriptions * Update deps and wrap setFontAppearance and resetFontAppearance in useCallback * Add e2e test for appearance control dropdown menu * Add periods to end of comments * Use better test data for font appearance e2e default test * Add findNearestFontStyle function with tests * Limit the dependency array to just fontFamily * Add normal font style test * Add invalid font style test * Try toHaveText instead of toContainText * Try using TT4 for e2e test rather than TT3 * Use combobox role rather than button in e2e test * Set nearestFontStyle to an empty string by default * Refactor findNearestFontStyle * Do not activate a theme in font appearance e2e test * Run findNearestStyleAndWeight only when fontFamily has changed * Only trigger appearance onChange if values are different Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: noisysocks <noisysocks@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
I just cherry-picked this PR to the release/18.8 branch to get it included in the next release: 72df623 |
…s#63215) * Refactor font appearance fallbacks * Create new findNearestStyleAndWeight function * Add tests for findNearestStyleAndWeight * Refactor findNearestStyleAndWeight * Add a test for normal/100 * Add comments * Tidy up comments * Fix test description * Update test descriptions * Update deps and wrap setFontAppearance and resetFontAppearance in useCallback * Add e2e test for appearance control dropdown menu * Add periods to end of comments * Use better test data for font appearance e2e default test * Add findNearestFontStyle function with tests * Limit the dependency array to just fontFamily * Add normal font style test * Add invalid font style test * Try toHaveText instead of toContainText * Try using TT4 for e2e test rather than TT3 * Use combobox role rather than button in e2e test * Set nearestFontStyle to an empty string by default * Refactor findNearestFontStyle * Do not activate a theme in font appearance e2e test * Run findNearestStyleAndWeight only when fontFamily has changed * Only trigger appearance onChange if values are different Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: noisysocks <noisysocks@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
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
findNearestStyleAndWeight
function 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