fix(pagination): simplify to single input with wrapped form and mobile submit#2861
Open
zeroedin wants to merge 13 commits intostaging/eeveefrom
Open
fix(pagination): simplify to single input with wrapped form and mobile submit#2861zeroedin wants to merge 13 commits intostaging/eeveefrom
zeroedin wants to merge 13 commits intostaging/eeveefrom
Conversation
zeroedin
requested review from
adamjohnson,
bennypowers,
coreyvickery and
hellogreg
🦋 Changeset detectedLatest commit: 61d0094 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for red-hat-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Size Change: +594 B (+0.22%) Total Size: 266 kB
ℹ️ View Unchanged
|
Contributor
Documentation Health
|
| Category | Score | |
|---|---|---|
| Element description | 25/25 | ✅ |
| Attribute documentation | 20/20 | ✅ |
| Slot documentation | 13/15 | ✅ |
| CSS documentation | 10/15 | |
| Event documentation | 15/15 | ✅ |
| Demos | 10/10 | ✅ |
Recommendations:
- rh-pagination: reference design tokens or theme considerations in CSS descriptions (CSS documentation, +4 pts)
- rh-pagination: mention accessibility considerations for slot content (Slot documentation, +2 pts)
- rh-pagination: add descriptions to all CSS custom properties (CSS documentation, +1 pts)
Collaborator
|
Noticed that the page number field always resets to 1 (and not the current page value) after submittal. This differs from production. pagination-on-submit.mp4 |
bennypowers
reviewed
Mar 8, 2026
Member
|
@hellogreg PTAL |
Assisted-By: Claude Opus 4.6 <noreply@anthropic.com>
5 tasks
bennypowers
approved these changes
Mar 9, 2026
Member
|
@coreyvickery please take a look and let us know if this needs any changes |
coreyvickery
requested changes
Mar 9, 2026
Collaborator
There was a problem hiding this comment.
@zeroedin Small things.
- Greg posted a video of something I see too, but it works as normal after a few tries
- Number boxes seem to be a tad bigger than the control boxes
- Input field with page number has no border (on landing it doesn't, but it does when I navigate to a different page)
- Controls (except for page input field tiny up/down arrows) don't work (not sure if that's a bug or not for this particular PR
- When the page number stuff is below the controls, reduce the padding to
--rh-space-xl - Mobile behavior and location of page number stuff looks good
hellogreg
requested changes
Mar 10, 2026
Collaborator
hellogreg
left a comment
hellogreg
left a comment
There was a problem hiding this comment.
Controls (except for page input field tiny up/down arrows) don't work (not sure if that's a bug or not for this particular PR
I found the same thing as Corey. All the arrow controls are disabled in all browsers I checked.
Collaborator
Author
oddly enough they work in the dev server. Taking a look at this. Update: was an SSR issue, fixed in the 68246a5 and 984d304 below |
hellogreg
reviewed
Mar 10, 2026
Collaborator
hellogreg
left a comment
hellogreg
left a comment
There was a problem hiding this comment.
One thing I've noticed in Firefox (at least Mac Firefox) is that clicking the up/down incrementer/decrementer does not bring focus to the number input. So, you can't increment/decrement and just press return. You have to increment/decrement, then click in the input, and then press return.
Not sure if this is a Firefox thing or an us thing.
Collaborator
Author
Was definitely a firefox thing, added code that now forces focus back on the input after the spinner is clicked. |
hellogreg
approved these changes
Mar 10, 2026
Collaborator
There was a problem hiding this comment.
Tested with mouse, keyboard, and screen reader:
- Mac Safari (VoiceOver)
- Mac Chrome (VoiceOver)
- Mac Firefox (VoiceOver)
- Windows Edge (Narrator)
- Windows Chrome (JAWS)
- Windows Firefox (NVDA)
Tested with touchscreen, onscreen keyboard, and screen reader:
- iOS Safari (VoiceOver)
- Android Chrome (TalkBack)
LGTM
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 I did
Replaces the closed branch #2846 to tidy up the changes.
Testing Instructions
Notes to Reviewers
@coreyvickery If additional design spec changes are necessary let me know. I know there was an outstanding question about how this moves forward with the unified theme by @markcaron.
@hellogreg when you have a moment give this a once over again in screen readers, not much if anything should have changed from the previous review in #2846 but just to be certain I didn't break anything in pulling those changes here.