Add support for RTL writing direction#1102
Add support for RTL writing direction#1102julesyoungberg wants to merge 10 commits intoFormidableLabs:mainfrom
Conversation
- Updated Carousel component to handle smooth scrolling behavior based on initial load state. - Introduced RTL support in useMeasurement hook for accurate scroll offsets. - Added isRTL utility function to determine text direction for proper layout handling.
- Improved Carousel component to prevent unnecessary scroll animations during initial load. - Updated useMeasurement hook to correctly handle negative scroll offsets in RTL mode. - Added utility function to detect RTL layout for better scroll behavior management.
- Consolidated RTL direction tests in browser utility to use parameterized tests for better readability and maintainability. - Simplified RTL offset tests in useMeasurement by utilizing a mock element and parameterized test cases for various scroll distances.
- Streamlined the parameterized test for RTL direction in browser utility by consolidating the test structure, enhancing clarity and maintainability.
- Introduced a new story for the Carousel component that demonstrates its functionality in a right-to-left (RTL) layout. - Added buttons for navigating between slides in the RTL context, enhancing the component's usability for RTL users.
- Updated the isRTL function to accept an optional HTMLElement parameter, allowing detection of RTL mode based on computed styles. - Enhanced the useMeasurement hook to utilize the updated isRTL function with the container element. - Added tests to verify RTL detection from element styles and inheritance from parent elements.
- Added JSDoc comments to the isRTL function to clarify its purpose and usage for detecting right-to-left (RTL) layouts. - Enhanced code readability and maintainability by providing detailed descriptions of the function's behavior.
- Simplified the scrolling logic in the Carousel component to enhance smooth scrolling after the initial load. - Removed unnecessary checks and streamlined the handling of the initial page load to prevent unwanted animations. - Ensured that the scroll behavior is consistent and responsive to the current page state.
🦋 Changeset detectedLatest commit: 5642edf 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 |
|
@julesyoungberg is attempting to deploy a commit to the formidable-labs Team on Vercel. A member of the Team first needs to authorize it. |
|
This is certainly needed. Thank you for taking it on! Any idea when this will be processed/merged/released? |
There was a problem hiding this comment.
Pull request overview
Adds RTL (right-to-left) support to nuka-carousel by detecting direction and adjusting computed scroll offsets so the Carousel can navigate correctly in RTL layouts.
Changes:
- Introduces
isRTL()browser utility + unit tests. - Updates
useMeasurementto generate RTL-aware scroll offsets. - Adds an RTL Storybook story and a changeset entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/nuka/src/utils/browser.ts | Adds isRTL() direction detection helper. |
| packages/nuka/src/utils/browser.test.ts | Adds unit tests for isRTL() behavior. |
| packages/nuka/src/hooks/use-measurement.tsx | Adjusts measurement logic to output RTL-aware scroll offsets. |
| packages/nuka/src/hooks/use-measurement.test.tsx | Adds tests validating RTL offset behavior via isRTL mocking. |
| packages/nuka/src/Carousel/Carousel.stories.tsx | Adds a Storybook story rendering the carousel under dir="rtl". |
| .changeset/tricky-parts-battle.md | Records the RTL support change as a patch release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (element) { | ||
| const direction = window.getComputedStyle(element).direction; | ||
| return direction === 'rtl'; | ||
| } | ||
|
|
||
| return document.documentElement.dir === 'rtl'; |
There was a problem hiding this comment.
isRTL()'s docstring says it checks computed styles for document-level direction, but the implementation only reads document.documentElement.dir (attribute) when no element is passed. This misses cases where direction is set via CSS or inherited (e.g., body { direction: rtl; }). Consider using getComputedStyle(document.documentElement).direction (and/or document.body) for the no-element path so behavior matches the comment and is more robust.
| if (element) { | |
| const direction = window.getComputedStyle(element).direction; | |
| return direction === 'rtl'; | |
| } | |
| return document.documentElement.dir === 'rtl'; | |
| if (element) { | |
| const direction = window.getComputedStyle(element).direction; | |
| return direction === 'rtl'; | |
| } | |
| if (document.documentElement.dir) { | |
| return document.documentElement.dir === 'rtl'; | |
| } | |
| const documentDirection = | |
| window.getComputedStyle(document.documentElement).direction || | |
| (document.body ? window.getComputedStyle(document.body).direction : ''); | |
| return documentDirection === 'rtl'; |
|
|
||
| if (element) { | ||
| const direction = window.getComputedStyle(element).direction; | ||
| return direction === 'rtl'; | ||
| } | ||
|
|
There was a problem hiding this comment.
There appears to be trailing whitespace on the blank lines after the isBrowser() check and before the final return. This can cause lint/prettier failures; please remove the extra spaces.
| if (element) { | |
| const direction = window.getComputedStyle(element).direction; | |
| return direction === 'rtl'; | |
| } | |
| if (element) { | |
| const direction = window.getComputedStyle(element).direction; | |
| return direction === 'rtl'; | |
| } |
| const rtl = isRTL(container); | ||
|
|
||
| switch (scrollDistance) { | ||
| case 'screen': { | ||
| const pageCount = Math.round(scrollWidth / visibleWidth); | ||
| let offsets = arraySeq(pageCount, visibleWidth); | ||
|
|
||
| // In RTL mode, scroll offsets must be negative (except for the first page at 0) | ||
| // because scrollLeft uses negative values to scroll right in RTL layouts | ||
| if (rtl) { | ||
| offsets = offsets.map((offset) => (offset === 0 ? 0 : -offset)); | ||
| } |
There was a problem hiding this comment.
RTL scrollLeft semantics are not consistent across browsers (some use negative values, others use positive descending/ascending). The current approach of simply negating offsets (and the comment asserting negative scrollLeft) will be incorrect in browsers that don't use the "negative" RTL model, causing navigation to scroll to the wrong positions. Consider normalizing RTL scrollLeft behavior (detect the browser's RTL scroll model at runtime and convert logical offsets to the correct scrollLeft values) instead of assuming negatives.
| ['ltr', false], | ||
| ['', false], | ||
| ['auto', false], | ||
| ])('should return %s when document direction is "%s"', (dir, expected) => { |
There was a problem hiding this comment.
The parameter placeholders in the it.each title are reversed: the table passes (dir, expected) but the title reads "should return %s when document direction is "%s"". This makes failing test output confusing; swap the placeholders or rename the parameters so the message matches what’s being asserted.
| ])('should return %s when document direction is "%s"', (dir, expected) => { | |
| ])('when document direction is "%s", should return %s', (dir, expected) => { |
Summary
useMeasurementhook to calculate negative scroll offsets for RTL layoutsisRTLutility function to detect text direction from element or documentDescription
This PR adds comprehensive RTL (Right-to-Left) language support to the Carousel component, enabling proper functionality for languages like Arabic and Hebrew.
Changes include:
useMeasurement: Enhanced the hook to detect RTL direction and calculate negative scroll offsets for RTL layouts. In RTL mode,scrollLeftuses negative values to scroll right, so all offsets (except the first page at 0) are negatedisRTLutility: Added utility function inbrowser.tsthat detects RTL mode from element computed styles or document direction attributedir="rtl"wrapper and navigation buttonsisRTLutility and RTL-specific behavior inuseMeasurementThese changes ensure the carousel works correctly for international users using RTL languages without modifying any existing carousel behavior.
Type of Change
How Has This Been Tested?
Unit Tests:
isRTLutility function (59 lines in browser.test.ts)useMeasurementhook (62 lines)Manual Testing Scenarios:
scrollDistanceset to 'screen', 'slide', and numeric valuesChecklist
pnpm run lint)pnpm run test:ci-with-server/pnpm run test)pnpm changeset.