Font Library: Simplify pagination and use chevron icons on font collection page #59591
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: -60 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
jasmussen
reviewed
Mar 5, 2024
There was a problem hiding this comment.
This is a big step forward. More work to be done, but this is close. I also think we can do without the first page / last page.
I believe you should be using the big default chevronLeft, rather than the small version, however, @jameskoster would know for sure.
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
Without a related issue, it is difficult to know what problem this PR solves, and if it accomplishes it. |
|
For example, what is the bug? |
|
The pagination controls in data views have been through a bunch of design iterations recently, so it could make sense to visually align with that: 
Ideally they'd use the same components, but I don't think it's been made reusable yet. This raises a more existential question for the future – should the font management experience be a data view?
The large variant is used in the UI above 👍 |
The main issue this PR addresses is replacing the unicode characters used in the pagination with chevron icons from the icons library. |
|
Right, the goal of this PR is to get us closer to having a single pagination component for all such UIs. The benefit of doing this is that the accessibility and design polish done to one component benefits all instances of said component. Since the |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
|
Given that RC1 is today, I'm removing this from 6.5 as non essential. |
Thanks for the clarification. However, I'm not sure removing the 'first' and 'last' makes usability any better.
I'm not sure this PR improves accessibility. While I do realize icons are better than unicode, this PR actually only changes the visuals. Also, I don't see any associated issue to allow broader discussion as requested by the contributing guidelines of this project. I'd kindly ask everyone to just follow the guidelines and please remind this is a collaborative open source project. Thanks. Overall, I would like to see more focus on #58133 rather than implemennting ad interim changes that don't actually solve the broader issue and risk to stay there for ages. |
|
Yes, I would actually like to see the first/last buttons kept. Changes to icons don't mean much to me as long as there are labels. A work-around might be to display the first and end page numbers. E.g. Previous, 1 (First), 2, 3, 4, 20 (Last), Next |
|
Thanks all. I'm not sure how best to proceed here and it sounds like there is some further discussion required around the ideal pagination controls. Perhaps that's worth discussing in #58133 and we could close this PR for now? |
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?
This simplifies the pagination of the font collection tab by removing the first and last page arrows, and uses the WordPress chevron icons in place of the current unicode characters.
Why?
It's best practice to use icons from the WordPress icons library where possible. After speaking with @jasmussen, the previous/next arrows should provide enough navigation for this component.
Testing Instructions
Screenshots or screencast