chore(Pagination): refactored to use MenuToggle instead of OptionsMenu#6607
Conversation
|
Preview: https://patternfly-pr-6607.surge.sh A11y report: https://patternfly-pr-6607-a11y.surge.sh |
| ``` | ||
|
|
||
| ## Documentation | ||
| Note: `<button>` or `<a>` elements can be used in `.pf-v6-c-pagination__nav-page-select`. |
There was a problem hiding this comment.
This wasn't accurate, as the element refers to the number input field for page select. We also don't allow customizing the elements for buttons used elsewhere in the component in React (menu toggle should be a button and the next/previous controls should be buttons)
| .#{$pagination}__nav { | ||
| display: flex; | ||
| flex-basis: 100%; | ||
| justify-content: space-between; |
There was a problem hiding this comment.
This was causing an issue where visual focus wasn't matching with tab order. The following screenshot makes it seems like pressing Tab would make the "previous" button on the left be focused first, then the menu toggle, but the menu toggle receives focus first then the "previous" button does. I can update this styling further (do we want it to look more like compact pagination where everything is pushed to the right, or do we want all controls centered?), but just wanted to remove this for now to at least fix the issue.
There was a problem hiding this comment.
Hmm.... I don't see a specific design in figma for bottom pagination on mobile - did you talk with design about this change? If not, IMO this is probably better as a separate issue to address the a11y of the tab order and update the design/layout.
For reference, this is what it looks like now
If we go with something like this, it's probably worth looking at the CSS some more to see if we can not use absolute positioning for the menu since the way that's written currently is specific to the old layout.
There was a problem hiding this comment.
@kaylachumley @lboehling wdyt? My screenshot is the existing design, Michael's screenshot is what it looks like with my updates in this PR. If we'd want something different from what is in this PR I can revert and open a followup to tackle this.
There was a problem hiding this comment.
I think the first example is a better interaction for mobile use -- separating the front and back arrows is better for touch interaction/a more typical layout for mobile!
There was a problem hiding this comment.
@thatblindgeye other than this comment, the PR looks good to merge 👍
There was a problem hiding this comment.
Opened https://github.com/patternfly/patternfly/issues/6731 as a followup
There was a problem hiding this comment.
LGTM, just a few nits, and larger comment around the layout change for bottom/mobile pagination. Just want to make sure design approves, and if there is more work to be done, probably doing that in a separate issue.
src/patternfly/components/Pagination/pagination-menu-toggle.hbs
Outdated
Show resolved
Hide resolved
src/patternfly/components/Pagination/pagination-menu-toggle.hbs
Outdated
Show resolved
Hide resolved
src/patternfly/components/Pagination/pagination-menu-toggle.hbs
Outdated
Show resolved
Hide resolved
| .#{$pagination}__nav { | ||
| display: flex; | ||
| flex-basis: 100%; | ||
| justify-content: space-between; |
There was a problem hiding this comment.
Hmm.... I don't see a specific design in figma for bottom pagination on mobile - did you talk with design about this change? If not, IMO this is probably better as a separate issue to address the a11y of the tab order and update the design/layout.
For reference, this is what it looks like now
If we go with something like this, it's probably worth looking at the CSS some more to see if we can not use absolute positioning for the menu since the way that's written currently is specific to the old layout.
9c8c986 to
42db8b2
Compare
42db8b2 to
b583bf2
Compare
|
🎉 This PR is included in version 6.0.0-alpha.159 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #6094
Pagination preview