Apply minimal variant to pagination dropdown #63815
Conversation
jameskoster
added
[Type] Enhancement
|
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: +3.7 kB (+0.21%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
I'm not sure that #63691 is much of an improvement without the other changes in this PR. The dropdown looks very disconnected from the prefix/suffix. Totally fine to close this and work on that PR instead though. |
| .components-select-control__input { | ||
| &:hover, | ||
| &:focus { | ||
| color: $gray-900; |
There was a problem hiding this comment.
cc @mirka when using the minimal variant of SelectControl, some wp-admin styles bleed through affecting the hover/focus color. I wonder if we should fix that in the component?
|
Would it be better to go ahead with this PR and add @dhananjaykuber to the props when it is merged? |
| __( 'Page ' ) + | ||
| page.toString() + | ||
| sprintf( | ||
| // translators: %s: Total number of pages. | ||
| _x( ' of %s', 'paging' ), | ||
| totalPages | ||
| ), |
There was a problem hiding this comment.
This configuration may cause problems with localization. Perhaps we could configure it like this:
sprintf(
// translators: 1: Current page number, 2: Total number of pages
__( 'Page %1$s of %2$s' ),
page.toString(),
totalPages
)
There was a problem hiding this comment.
Now that #63855 has been merged, can we remove this CSS after rebasing this PR?
By the way, I'm considering applying the approach taken in this PR to the Font Library Modal as well. In that case, it will look like this, but is this as expected? The font library has a particularly large number of pages.
| Before | After |
|---|---|
 |
 |
|
Yeah that's not ideal. Perhaps we can do something with the |
|
Using the suffix didn't really work and resulted in worse usability (the click target was tiny). This is the closest I can get to the intended design, by using the inline label: 
But it's not perfect. "Page" is omitted from the 
We might need to rethink the design a bit. |
|
Is the problem here that If so, it looks like we should just delete this On my machine it looks like this:
By the way, the option itself with the text "1 of x" seems fine. This text is also used in the core: |
|
If we delete that line then the text in the I'm trying another approach using the 
I think this might thread the needle, though there's a small bug; clicking the suffix doesn't open the |
The prefix and suffix are just elements that exist alongside the select element, but they visually exist inside the select box, so this behavior seems like a bug to me too. 253ba79e7d56dea65fdb587d5ed453b0.mp4 |
|
If we can fix that (likely a separate endeavour) then I reckon this PR is in a decent spot in terms of design. |
There was a problem hiding this comment.
Before getting into the design specifics, I want to figure out the best way to deal with the labeling.
Using the suffix is not ideal because we can't allow click through to the <select> if the suffix has a dynamic width (it only works with the caret because the caret is fixed-width). Also __( 'of %s pages' ) is not the best for localization because the word order in a phrase like "x of y pages" can invert the x and y depending on the language.
I'm wondering if this kind of implementation would satisfy our requirements better and with a simpler implementation. Let me know if I'm missing some requirements.
|
@mirka Thank you for your feedback. I looked at your sample and your approach of showing "Page %s of %s" for the currently selected option and just showing the page number for the other options seems like a good idea. It also works well for screen readers: 5c51cd5ef588c30cc8f06c4ee860f3c5.mp4 |
|
Yes I think that might work. But we'll still run into the styling issue where the size and capitalisation are applied to the |
|
I switched over the implementation. I applied the most basic styling that doesn't require hacking around the component styles, but please tweak as you see fit 👍 
|
|
I added the one translation function that was missing, but it appears to be working as expected 👍 |
packages/dataviews/src/components/dataviews-pagination/index.tsx
Outdated
Show resolved
Hide resolved
Is this an issue on trunk? I'm just wondering if the default could be |
Good idea, I think that could work. Added in ac39701 
|
@t-hamano Thank you 🙈 |
|
Looking good, thanks @mirka. I think the last question is whether we want to apply the same text styles as trunk: 
Now that the 
What do you think @jasmussen ? |
| sprintf( | ||
| // translators: %s: Total number of pages. | ||
| _x( 'Page <CurrentPageControl /> of %s', 'paging' ), | ||
| <div aria-hidden>{ __( 'Page' ) }</div> |
There was a problem hiding this comment.
Why we removed createInterpolateElement here? It feels like the current approach for translation is not ideal.. I'm not 100% sure if the current approach is right, but I was wondering why it was needed? I'll cc @swissspidy if he has any suggestion.
There was a problem hiding this comment.
Yeah I wouldn't remove it. Doing something like __( 'of %s' ) makes this worse. createInterpolateElement is important here to avoid such string concatenation.
I would even improve the translatable string a little bit like this:
sprintf(
/* translators: 1: Current page, 2: Total pages. */
_x( '%1$s of %2$s', 'paging' ),
'<CurrentPageControl />',
totalPages
)
|
Works for me, though I'd refine that minimal dropdown a bit more. The space between the 1 and the down-chevron is pretty side at the moment, and since the 1 is included in the tap target, it doesn't need to be that wide. I don't know that's a blocker for this PR, perhaps it is? A quick stab:
That removes 5px left/right padding, and 5px gap between them. We can find a rounder number than 5 (maybe 4 or 6), but this was a quick photoshop just to share the thought. |
|
Another thing I noticed while testing this PR is that when 84e32eb99d406b731d606a93c216351d.mp4This can also be reproduced in Storybook: 36da88c6edbc2f80ea5f6f9a00396bdf.mp4For example, in the Font Library Modal's Install Fonts tab, there are 200 pages of installable fonts, so if we apply the pagination style of this PR, the change in the select box width will be more noticeable. |
Agree, but let's make that change separately in the component. @t-hamano the change in width is intentional, otherwise the chevron can appear far away from the selected value. This makes it harder to recognise the element as a select. |
|
How are we looking here? :D |
There was a problem hiding this comment.
From a technical standpoint this is good to go 👍 As for styles, I'll defer to the design folks — please push whatever tweaks are deemed necessary before landing. Though I do recommend we figure out a style that works for all instances of the minimal SelectControl variant, rather than ad hoc it just for this one. This pagination UI is one of the very few use cases for the minimal variant, and if it doesn't work here then it will probably not work in other places as well.
|
I think the only necessary overrides are the font size / weight. It's not yet clear whether that's something we'll need at the component level. I pushed those changes and will update the OP. There are also minor tweaks to consider, but I think we can do those separately. |
There was a problem hiding this comment.
I think it's okay to ship this PR. From what I understand, I would expect this pagination style to apply to all other paginators below, but what about you?
- Font Library
- Global Styles Revision
- Pattern Modal
- Pattern Inserter
By the way, although it is not directly related to this PR, I found that the SelectControl component is affected by the line-height style of WP-Admin. Visually, it is a small difference, but I think this issue needs to be fixed separately:
Edit: I found that this issue occurs on the combination of Windows OS and Chrome. The issue might not occur on other OS or browsers.
| @@ -11,11 +11,18 @@ | |||
| @include reduce-motion("transition"); | |||
| } | |||
|
|
|||
| .dataviews-pagination__page-selection { | |||
| .dataviews-pagination__page-select { | |||
| color: $gray-900; | |||
There was a problem hiding this comment.
If we remove this style, the color override appears not to be necessary:
Ideally, yes. |
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: swissspidy <swissspidy@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
What?
Apply
minimalvariant to pagination control in data views.Why?
The current implementation gives outsized importance to this element.
Testing Instructions
I suspect there's a better way to handle the i18n, so please feel free to push to this branch :)