fix(pagination): replace onchange with onsubmit#2846
Closed
zeroedin wants to merge 13 commits intostaging/eeveefrom
Closed
fix(pagination): replace onchange with onsubmit#2846zeroedin wants to merge 13 commits intostaging/eeveefrom
zeroedin wants to merge 13 commits intostaging/eeveefrom
Conversation
🦋 Changeset detectedLatest commit: bea8787 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: +76 B (+0.03%) Total Size: 261 kB
ℹ️ View Unchanged
|
Contributor
Documentation Health
|
| Category | Score | |
|---|---|---|
| Element description | 11/25 | |
| Attribute documentation | 20/20 | ✅ |
| Slot documentation | 0/15 | ❌ |
| CSS documentation | 4/15 | ❌ |
| Event documentation | 15/15 | ✅ |
| Demos | 5/10 |
Recommendations:
- rh-pagination: use RFC 2119 keywords (MUST, SHOULD, AVOID) to clarify requirements (Element description, +5 pts)
- rh-pagination: add descriptions to all slots (Slot documentation, +5 pts)
- rh-pagination: describe expected content types for slots (e.g., 'inline text', 'block elements') (Slot documentation, +5 pts)
- rh-pagination: mention accessibility considerations for slot content (Slot documentation, +5 pts)
- rh-pagination: add descriptions to all CSS parts (CSS documentation, +5 pts)
zeroedin
changed the title
bennypowers
approved these changes
Feb 26, 2026
Member
bennypowers
left a comment
bennypowers
left a comment
There was a problem hiding this comment.
Lucid "go" text manifested
Member
|
Does this need l10n? |
Collaborator
Author
As far as I understood its a keyword only and isn't exposed to the client. https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/enterKeyHint I asked gemini just for the check:
|
Member
|
|
adamjohnson
approved these changes
Feb 26, 2026
Collaborator
adamjohnson
left a comment
adamjohnson
left a comment
There was a problem hiding this comment.
Tested on Android 16 with Chrome and Opera Mobile. Works as advertised. Nice job.
hellogreg
approved these changes
Feb 27, 2026
Collaborator
hellogreg
left a comment
hellogreg
left a comment
There was a problem hiding this comment.
Working with iOS VoiceOver and Android Talkback 🙌🏻
…eObserver for correct focus order
Collaborator
Author
|
noticed URLs aren't actively updating. I didn't notice that the form was being rendered twice to get around focus order when resized to the < 768px breakpoint view.
|
Collaborator
Author
|
In a full reversal again I went back to the resize observer. In order to properly link the |
adamjohnson
requested changes
Mar 2, 2026
Collaborator
adamjohnson
left a comment
adamjohnson
left a comment
There was a problem hiding this comment.
Looking at the base demo currently (demo/index.html):
- Where we have "Page {input} of 5", the "of 5" bit should stay on the same line with the input but it currently wraps to the next line.
- The height of the input does not match the height of the rest of the pagination buttons.
- If I enter "3" into the input and hit enter, pagination correctly adds
aria-current="page"to the third item in the ordered list, but the input displays "Page {1} of 5" instead of "Page {3} of 5" - At 330px, the content inside
#numericdoesn't have any margin/gap to separate it from the<<|<|>|>>buttons- Note: these breakpoint numbers will likely change when fixing the wrapping issue mentioned above.
- At 344px+,
#compact-numericis also missing its margin/gap on the right side
And, as you know, there are a few failing tests to resolve. 😇
Collaborator
Author
|
Closing this PR, will open a new one with some of the changes here, but in the direction of modifying the design by not doing the shift of the form. Ran into different issues with each attempt and want to start with a fresh take on the problem. |
github-project-automation
bot
moved this from Review 🔍
to Done ☑️
in Red Hat Design System
Member
|
Thanks for your intensive work on this one, @zeroedin |
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
<form>for mobile submit supportenterkeyhint="go"attribute replacinginputmode="numeric"giving the mobile keyboard a enter key the represents a navigation action.TODO
Testing Instructions
enterkeyhint="go"should display a navigation style enter key.Notes to Reviewers
Please look at mobile browser if you have android phone I do not so I can't test the enterkeyhint there.
This change does change the user expectation that the input field change auto submits via onChange -> onSubmit which needs the user to press enter or hit the "go" button? Does we think this constitute enough for a major change? Feel it maybe in a grey area.