Pagination jump to page - #4306
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: Lundalogik/lime-elements/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4306/ |
0bd018e to
bddf9ce
Compare
Consolidated PR ReviewPR SummaryReview tier: Full — 1271 changed lines across 21 files after noise filtering (nothing was filtered out; no security-sensitive paths). Seven agents ran: Backward Compatibility, Code Quality, Observability and Performance on The PR turns the Since the previous review the branch has answered all four of that review's Top Recommendations and five of its nine nits. Of the four raised this round — one High and three Mediums, all in new code — three are fixed and one is deferred to #4308, because its fix is a new method on Merge Readiness — READY TO MERGE ✅ (was BLOCKED 🛑)The blocker is gone, and not by regenerating the API report. Top Recommendations 1 and 2 turned out to be one question — whether One Medium is deferred rather than fixed: reading the typed value out of
DimensionsShort summaries. Each issue points to a numbered Top Recommendation below. 1. Backward Compatibility — GOOD ✅ (was PROBLEM ❌)Nothing a consumer of Issues:
What works well: Same props, same Minor nits:
2. Code Quality — NEEDS ATTENTION
|
A gap carried `from` and `to` so that a jump-to-page control could offer the pages it hides. That control is now unscoped: both ellipses open the same field and reach any page in the set, because two identical markers that silently accept different ranges cannot be told apart by the user. Nothing reads the range, so it goes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`limel-pagination-jump` is a number field and a button for going straight to a page. It is what `limel-pagination` will put inside the popover behind its `···`, and it is private, so nothing here is a promise to anyone outside this repository. It is a component rather than markup inside the pagination because a popover carries whatever it holds into a shadow root elsewhere on the page, where the stylesheet of the component that opened it cannot reach. A component brings its own, and gets its own tests and example page with it. It draws nothing at all until it is opened, so a pagination merely on screen pays for an empty element rather than a Material text field per gap. It reads the page out of the field when asked to commit, rather than from the field's `change` event, which arrives on a 300 ms debounce: a page typed and submitted inside that window would otherwise be submitted as whatever preceded it. What it shows still follows that event, because the field draws an unfocused number from its `value` prop, and a field pinned to the page it opened with would put the old number back the moment the keyboard moved to the button. A page outside the set is not refused. The set has a first and a last page, both written in the field, and the nearer of them is what someone typing past the end meant; `pageFromInput` is that rule, kept pure and tested on its own. It takes a count of `null` because a set whose size is unknown is a real state; the component does not, because a jump field with no upper bound cannot name the set it is jumping within. Its tests run in a browser rather than against the mock DOM, which cannot host a Material text field inside another component. They drive the component on its own, with no popover in the way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mock-doc does not implement the `slot` IDL property, so an element written as `slot="trigger"` reads back `undefined`. A component that routes its children by `element.slot` then behaves differently under test than in a browser: `limel-popover-surface` keeps a trigger out of its portal that way, and without the property it swallowed the trigger instead, leaving nothing in the component to click. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A gap becomes a button that opens `limel-pagination-jump` in a popover, so no page is more than one move away however long the set is. Enter commits, Escape closes, and the marker shows a pressed look while its field is open, so one of two identical markers can be seen to be the one that was opened. Both gaps open the same field, and it reaches any page in the set. Scoping each one to the pages it happens to hide would make two identical markers behave differently, with nothing on screen to say which is which, and would turn away someone who wanted page 3 because they opened the marker on the right. The component is controlled, so committing only asks: it emits `goToPage` and waits. That decides where the keyboard goes. On commit the field closes and focus returns to the marker it came out of, which is still on screen and is where it stays if the consumer declines to show the page. When the page does arrive, focus follows to the button for it, which carries `aria-current`, so a screen reader says where it landed. If the user moved the keyboard themselves in the meantime, it is left alone; and if the consumer answers by showing some other page, the jump is given up, so a later unrelated arrival at the page once asked for does not move the keyboard as though it were the reply. Two states needed guarding. A gap is remembered by the slot it sits in, and a page set from outside can put a page number in that slot, taking the popover down without it ever closing — left open, it would spring back the next time a gap landed there. And the marker holding the keyboard can itself be taken away, which a browser answers by focusing the body rather than nothing, so that is what the follow step looks for. `goTo` becomes the one path every move the user asks for goes through, and so the one place that refuses while a page is being fetched, rather than each handler keeping its own check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5c04b6a to
c83cdfa
Compare
|
🎉 This PR is included in version 40.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Turns the
···inlimel-paginationinto a button that opens a small form for going straight to a page. Closes #4298.The control
Each gap becomes a button with an accessible name, showing the pressed look while its form is open so that one of two identical markers can be seen to be the one you opened. The form holds a number field, starting from the page you are on, and a Go button. Enter commits, Escape closes, clicking outside dismisses.
Both gaps open the same form, and it reaches any page in the set. Scoping each gap to the pages it happens to hide was considered and rejected: two markers that look identical would behave differently, with nothing on screen to say which is which, and someone who wanted page 3 would be turned away by the gap on the right for no reason they could see.
A page outside the set is not refused, it moves to the nearest page that exists. The set has a first and a last page, both written in the field, so someone typing past the end meant the end. Refusing would leave them to work out a bound the control already knows.
Asking, not moving
The component is controlled, so committing only emits
goToPage— the control does not move until you setpage. That shapes the focus behaviour, which is the part most worth reviewing:On commit the form closes and the keyboard goes back to the marker it came out of, which is still on screen. If you decline the request — by not setting
page, which is the documented way to decline — focus stays there and nothing has moved. When the page does arrive, focus follows to the button for that page, which carriesaria-current="page", so a screen reader reads out that it arrived. If the user moved focus themselves while waiting, it is left alone.A component of its own
The form is
limel-pagination-jump, a private component, rather than markup insidelimel-pagination. A popover carries its content into a shadow root elsewhere on the page, where the stylesheet of the component that opened it cannot reach — the alternative was laying the form out with inlined styles. A component brings its own stylesheet through the portal, and gets its own tests and example page.limel-pagination.tsxis about 70 lines smaller as a result.It draws nothing until it is opened, so a pagination that is merely on screen pays for an empty element rather than a live text field per gap.
How it is tested
Three layers, each where it can actually be exercised:
pageFromInput, a pure function inpagination.util.ts, table-tested for clamping, garbage, decimals and magnitudes past2**53..spec.tsxbecause the mock DOM used for spec files cannot host Material's text field inside another component — it recurses into MDC's focus handling and overflows the stack. That was confirmed to be the environment and not the code, by disabling focus and by removing the value feedback: it still crashed.goToPage, and the focus behaviour above — is tested againstlimel-paginationin a browser, where the portal exists.The full suite passes: 2493 tests across 140 files.
Shared fixes
This branch currently also carries the three
limel-popover/limel-menufixes that came out of building it. They are up for review on their own in #4307 and will drop out of here when this is rebased on top of it, so please review them there rather than in this PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: