fix(Pagination): mirror default control icons in RTL via CSS#6767
fix(Pagination): mirror default control icons in RTL via CSS#676761021 wants to merge 2 commits into
Conversation
The first/prev/next/last icons were swapped in JS based on useLocale().dir, while the list layout follows the DOM direction. When RTL comes from htmlAttrs.dir or UApp's dir prop without a locale, the layout flips but the icons don't, leaving the controls pointing inward. Keep the LTR default icons and mirror them with rtl:-scale-x-100 so icons and layout react to the same source of truth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/runtime/components/Pagination.vue (1)
153-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd RTL regression coverage for pagination controls.
Test default and custom first/previous/next/last icons under
dir="rtl"and verifyrtl:-scale-x-100reaches the leading icon, while slot content remains unaffected.Also applies to: 200-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/components/Pagination.vue` around lines 153 - 172, Add RTL regression coverage for the Pagination controls around the first/previous and corresponding next/last button render paths. Under dir="rtl", verify default and custom first, previous, next, and last icons receive the rtl:-scale-x-100 leading-icon styling, while slotted control content does not receive or alter that styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/runtime/components/Pagination.vue`:
- Around line 153-172: Add RTL regression coverage for the Pagination controls
around the first/previous and corresponding next/last button render paths. Under
dir="rtl", verify default and custom first, previous, next, and last icons
receive the rtl:-scale-x-100 leading-icon styling, while slotted control content
does not receive or alter that styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a483b32b-4dca-4206-a22a-4533cb0ece87
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Pagination-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Pagination.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/runtime/components/Pagination.vue
commit: |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/components/Pagination.spec.ts`:
- Around line 59-76: Update the “mirrors default control icons in RTL” test to
mount Pagination with an RTL direction context, then provide explicit firstIcon,
prevIcon, nextIcon, and lastIcon props and verify those custom controls receive
the expected RTL mirroring behavior. Retain the ellipsis assertion and add
coverage for the relevant icon rendering/accessibility contract without relying
only on the static class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 25c28503-c3ca-4345-bb21-1f3097844dfd
📒 Files selected for processing (1)
test/components/Pagination.spec.ts
| it('mirrors default control icons in RTL', async () => { | ||
| const wrapper = await mountSuspended(Pagination, { | ||
| props: { | ||
| total: 100, | ||
| page: 5, | ||
| showEdges: true, | ||
| siblingCount: 1 | ||
| } | ||
| }) | ||
|
|
||
| for (const control of ['first', 'prev', 'next', 'last']) { | ||
| const icon = wrapper.find(`[data-slot="${control}"] [data-slot="leadingIcon"]`) | ||
| expect(icon.classes(), `${control} icon`).toContain('rtl:-scale-x-100') | ||
| } | ||
|
|
||
| const ellipsisIcon = wrapper.find('[data-slot="ellipsis"] [data-slot="leadingIcon"]') | ||
| expect(ellipsisIcon.classes(), 'ellipsis icon').not.toContain('rtl:-scale-x-100') | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make this a real RTL regression test.
The test never mounts Pagination in an RTL context; it only checks the static rtl:-scale-x-100 class, which is applied regardless of direction. Add dir="rtl" to the mount context and cover explicit firstIcon, prevIcon, nextIcon, and lastIcon props, since custom icons are also part of the PR contract.
As per coding guidelines, component tests should cover relevant props, slots, and accessibility behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/components/Pagination.spec.ts` around lines 59 - 76, Update the “mirrors
default control icons in RTL” test to mount Pagination with an RTL direction
context, then provide explicit firstIcon, prevIcon, nextIcon, and lastIcon props
and verify those custom controls receive the expected RTL mirroring behavior.
Retain the ellipsis assertion and add coverage for the relevant icon
rendering/accessibility contract without relying only on the static class.
Source: Coding guidelines
1 participant
🔗 Linked issue
Resolves #6438
❓ Type of change
📚 Description
The first/prev/next/last icons are swapped in JS based on
useLocale().dir, while the list layout follows the DOM direction (dirattribute + logical properties). The two disagree whenever RTL is set throughhtmlAttrs.dir(as in #6438) or throughUApp'sdirprop without also passing alocale: the layout mirrors but the icons keep their LTR glyphs, so the controls point inward.This is reproducible in the playground itself: set
dir: 'rtl'inplaygrounds/nuxt/app/app.config.ts(it wireshtmlAttrs.dirandUApp'sdir, but no locale) and open/components/pagination.Following the RTL guideline added in #6724 (direction-dependent transforms get an
rtl:counterpart), this keeps the LTR default icons and mirrors them withrtl:-scale-x-100, so the icons and the layout react to the same source of truth and can't disagree — with a locale, without one, or inside adir="rtl"subtree.Notes:
first-icon/prev-icon/next-icon/last-iconprops are mirrored the same way (the component managed direction for them before too). Slot content is not affected.useLocale().diricon swap exists inBreadcrumb,Calendar,Carousel,ContextMenu/DropdownMenu(submenu icon) andContentSurround— happy to align those in a follow-up PR if this approach looks right.📝 Checklist