Font Library: Replace infinite scroll by pagination #58794
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: -1.59 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
pbking
added
[Focus] Accessibility (a11y)
matiasbenedetto
requested changes
Feb 8, 2024
There was a problem hiding this comment.
Wow, this is looking very nice :).
I have found 2 issues while testing:
- After the first pages, some of the preview images don't load.
- If you select a page from the dropdown component and click next, you navigate to page 1, and the list gets blank.
An screencast featuring both problems:
2024-02-08.12-35-54.mp4
matiasbenedetto
changed the title
Thank you @matiasbenedetto I have addressed that. (The select was setting the value as a string, though I'm not sure why. Easy enough to fix.) Regarding this:
I'm not sure that it's the changes here that are causing that. I've noticed the previews failing to load on most any branch I'm on lately. I'm not entirely sure why. It isn't currently reproducing for me (on this branch) though it WAS just a short while ago on another. |
|
Flaky tests detected in 7d28fa6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7835496492
|
t-hamano
reviewed
Feb 9, 2024
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Outdated
Show resolved
Hide resolved
afercia
added
[Type] Bug
|
Raising the Type label from |
carolinan
reviewed
Feb 9, 2024
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Outdated
Show resolved
Hide resolved
|
When I navigate with the keyboard and I activate the button that is meant to take me to the last page, it seems the focus is lost. |
This problem can be solved by adding |
This was addressed in this commit with the suggestion from @t-hamano |
t-hamano
reviewed
Feb 9, 2024
| <Button onClick={ () => setPage( 1 ) } disabled={ page === 1 }> | ||
| << | ||
| <Button | ||
| aria-label={ __( 'first page' ) } |
There was a problem hiding this comment.
It would be better to use label, which is an API of the Button component, instead of aria-label.
There was a problem hiding this comment.
Changed buttons to use label. Kept select component using aria-label so that the label wasn't added above the control.
t-hamano
reviewed
Feb 9, 2024
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Outdated
Show resolved
Hide resolved
|
I'm seeing the pagination available on the 'Install Fonts' screen before I've even consented to Google Fonts? 🤔 |
|
Thank you @colorful-tones for pointing that out. I've pushed a fix. |
|
Confirming that the pagination no longer display if user has not accepted and opted into Google Fonts. 👍 Overall, this is a wonderful improvement and I think the pagination is a better user experience for users. I have a few follow up discussion items, but I would say that they're not blockers for this work to be merged. I'm happy to open a new issue or log somewhere else. For now, here is my list:
Some nice guidance on pagination here: https://designsystem.digital.gov/components/pagination/ |
matiasbenedetto
approved these changes
Feb 9, 2024
There was a problem hiding this comment.
If you select a page from the dropdown component and click next, you navigate to page 1, and the list gets blank.
✔️ I can confirm that this is solved.
After the first pages, some of the preview images don't load.
This seems to be unrelated to these changes.
It would be excellent to add the accessibility improvements listed by @colorful-tones in this comment, but that doesn't seem to be mandatory for this PR.
It is testing fine on my end. I tested:
✔️ Page sizes dynamically adapt to the space available.
✔️ Navigating a font family works.
✔️ Going inside/out a font family keeps you on the same page.
✔️ Navigate to pages using buttons and dropdown components work,
✔️ Category and search filters continue working as expected.
LGTM.
pbking
added
the
Backport to WP 6.7 Beta/RC
This is caused by the google fonts collection being registered in core and in Gutenberg. That was already solved in Gutenberg trunk I think rebasing this branch will solve the problem in this branch too. |
…odal/font-collection.js Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
…odal/font-collection.js Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
…odal/font-collection.js Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
…odal/font-collection.js Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
pbking
force-pushed
the
try/font-collection-list-pagination
branch
from
4574dce to
b5f5984
Compare
t-hamano
approved these changes
Feb 13, 2024
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Outdated
Show resolved
Hide resolved
…odal/font-collection.js Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
|
I just cherry-picked this PR to the more/backports-for-beta3 branch to get it included in the next release: 3c9d2cd |
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
* Eliminate and render fonts with pagination instead of as a large list Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: okmttdhr <okat@git.wordpress.org>
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
added
the
Backported to WP Core
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
* Eliminate and render fonts with pagination instead of as a large list Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: jasmussen <joen@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?
Removes Infinite Scrolling behavior from Font Collection tab of Font Library Modal
Fixes: #58067
(Also Fixes: #58799)
Why?
Infinite scrolling behavior is not accessible for keyboard users.
How?
Calculates how many units (Font Family list items) can be rendered on the page and render only that many pages.
Adds X of Total (with X as a SELECT element) and start, previous, next, last buttons.
Testing Instructions
Navigate to the 'Install Fonts' tab
Note that rather than being presented with a scrollbar pagination buttons are presented in the modal's footer
Ensure the pagination controls work as you would expect.
While using the pagination buttons filter the lists (by category, and by name)
Select Font Families with more Font Faces than will fix without scrolling (Advent Pro is a good example).
Note that a scrollbar is used for the Font Faces.
Testing Instructions for Keyboard
Same Steps as normal instructions. The entire workflow should be achievable exclusively with the keyboard.