Use NavigatorProvider in Font Library Modal #59036
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. |
|
Size Change: -84 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 196a4bf. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7993941391
|
matiasbenedetto
added
Backport to Gutenberg RC
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
matiasbenedetto
added
[Focus] Accessibility (a11y)
pbking
force-pushed
the
try/font-library-modal-navigator
branch
from
2a72b0b
to
c8778fe
Compare
pbking
added
[Type] Bug
|
Thanks for working on this. This PR seems to be also fixing #54431 |
|
Sorry, I’m on vacation until 2/27. This looks promising at first glance, but I will not have the ability to spin it up in a dev server right now. I hope other’s will test and provide feedback to move this along. |
Silly me. That was the issue that started it all. I've added it to the fixes list. |
There was a problem hiding this comment.
The <FontDemo> component now expects a font family, and here we are passing a font face. It causes all the variants of a font family to render with the same default style (400, normal) when no preview is provided.
See the comparison:
| trunk | this branch |
|---|---|
 |
 |
Apart from the bug, I'm not sure that making this component smarter (understanding a more complex data structure as fontFamily instead of the plain fontFace) is needed or wanted. I would lean toward maintaining that component's ability to receive only font faces as a prop.
To reproduce the bug, upload a font family with several font variants using the 'Upload' tab.
Thank you @matiasbenedetto. I pushed a small change to better support accepting EITHER a font FACE or a font FAMILY for the demo. Result: 
To test I changed the default font collection to |
|
I think we should not use a custom font on the title, because it can be problematic:
|
It's equally problematic for the list you selected the font family for. If it's a problem as the title here it's a problem in the list and we should probably have some way around that in both places. |
We should probably find a workaround to the list of fonts, too, in a follow-up. But using the demo in that title is not in the mockups, and I don't think it is wanted, and it should not be added at this point either. |
Ok, you got me with that one. :) I've changed it back to a heading. |
…Font Card to handle navigator transitions and consolidated components when possible
…S in the Font Demo component.
pbking
force-pushed
the
try/font-library-modal-navigator
branch
from
dc603fb
to
aa0b118
Compare
|
@pbking, could you check why this behavior of the margins is happening in this branch? 2024-02-23.12-55-52.mp4 |
Ohh it's because of the revoke permissions buttons. That's unrelated to the scope this PR. Added a comment in #59205 (comment) |
|
I think PR introduced the following regression: when you click a font from the list, the modal opens in the main library tab and not on the tab listing the variants of the font family clicked. Opening the list of variants of the font clicked in the sidebar was one of the original requirements of the design, so we should restore that behavior. Screencast featuring behavior after this PR: 2024-02-26.13-18-18.mp4 |
9 tasks
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
* Use NavigatorProvider in installed fonts panel * Style Changes to font modal installed font details page * Simplified Font Face Demo component so it can be more easily reused * Refactored 'installed-fonts' component to use Navigator Components * Refactored Font Collection tab to use Navigator Provider, refactored Font Card to handle navigator transitions and consolidated components when possible * Corrected/standardized Font Demo component name throughout * Fixed notice display * Fixed focus state outline * Fixed secondary page heading layout * Added hover state to font list items * Simplified and removed 'TabPanelLayout' component * close verify window as soon as user ansswers * Fixed a bug to better support BOTH font FAMILIES as well as font FACES in the Font Demo component. * Returned selected font heading to use Heading instead of Font Demo * Fixed linting error Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: joedolson <joedolson@git.wordpress.org> Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org> Co-authored-by: afragen <afragen@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: juanfra <juanfra@git.wordpress.org> Co-authored-by: okmttdhr <okat@git.wordpress.org>
getdave
added
Backported to WP Core
|
I just manually cherry-picked this PR to the cherry-pick-wp-6-5-beta-3 branch to get it included in the next release. |
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
* Use NavigatorProvider in installed fonts panel * Style Changes to font modal installed font details page * Simplified Font Face Demo component so it can be more easily reused * Refactored 'installed-fonts' component to use Navigator Components * Refactored Font Collection tab to use Navigator Provider, refactored Font Card to handle navigator transitions and consolidated components when possible * Corrected/standardized Font Demo component name throughout * Fixed notice display * Fixed focus state outline * Fixed secondary page heading layout * Added hover state to font list items * Simplified and removed 'TabPanelLayout' component * close verify window as soon as user ansswers * Fixed a bug to better support BOTH font FAMILIES as well as font FACES in the Font Demo component. * Returned selected font heading to use Heading instead of Font Demo * Fixed linting error Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: joedolson <joedolson@git.wordpress.org> Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org> Co-authored-by: afragen <afragen@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: juanfra <juanfra@git.wordpress.org> Co-authored-by: okmttdhr <okat@git.wordpress.org>
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 and eliminates many view components to simplify and consolidate.
Use
NavigatorProvidercomponent for panels that drill into details.Fine-tuned keyboard navigation
Added hover styles
Refactored "Font Preview" component and used it in additional places
Why?
Simplifies the View Code
Implements Navigator pattern to address a11y navigation concerns
Addresses a number of Accessibility concerns
Fixes: #58893
Fixes: #58800
Fixes: #58386
Fixes: #54431
Testing Instructions
Ensure that all of the interactions (including error conditions) work as expected including:
Navigating to and installing fonts from the default collection
Installing fonts by uploading them
Deleting installed fonts
Activating and deactivating installed fonts
Activating and deactivating fonts provided by themes
Testing Instructions for Keyboard
Keyboard interactions are of primary concern for these changes. Please ensure that all of the actions are easily complete-able with keyboard use. A complete outline of keyboard steps are too vast to note here.
Screenshots or screencast