[WB-2162] Floating: Add RTL support #2881
Conversation
🦋 Changeset detectedLatest commit: 8ece8c8 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 |
|
Size Change: +154 B (+0.14%) Total Size: 112 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-bwwefbhhuc.chromatic.com/ Chromatic results:
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
marcysutton
left a comment
marcysutton
left a comment
There was a problem hiding this comment.
This looks good to me! Just left a few questions for my own understanding.
| }, | ||
| render: function Render(args, {globals}) { | ||
| const isRTL = globals.direction === "rtl"; | ||
| const placements: Array<{name: Placement; icon: PhosphorRegular}> = [ |
There was a problem hiding this comment.
praise: this is a slick way to set up these variants!
| gridTemplateColumns: "repeat(3, 1fr)", | ||
| gridTemplateRows: "repeat(4, 150px)", | ||
| placeItems: "center", | ||
| width: "100%", |
There was a problem hiding this comment.
question: Should we move to inlineSize and blockSize instead of width and height?
|
|
||
| describe("rtlMirror", () => { | ||
| // Helper function to create a reference element with optional RTL container | ||
| const createReferenceElement = (isRTL: boolean) => { |
There was a problem hiding this comment.
question: Is it possible this would be a useful helper function for other tests? Would it make sense to put it in a more general location?
There was a problem hiding this comment.
This is only used here for now. I'll move it if we start needing it for other test files.
| } | ||
|
|
||
| return { | ||
| reset: { |
There was a problem hiding this comment.
question: Is reset a @floating-ui/react thing? What does it represent?
There was a problem hiding this comment.
Good question, I'll add a comment + link to the docs :)
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Looks good to me! Left some non-blocking questions 😄
| placement, | ||
| elements: {reference}, | ||
| } = state; | ||
| const isRtl = !!(reference as Element).closest("[dir='rtl']"); |
There was a problem hiding this comment.
Since we cast the reference as an Element, Is it possible for reference to be null or not yet defined at runtime? I'm wondering if we need to check it before calling closest!
| if (placement.startsWith("left")) { | ||
| nextPlacement = placement.replace("left", "right") as Placement; | ||
| } else { | ||
| nextPlacement = placement.replace("right", "left") as Placement; |
There was a problem hiding this comment.
Do you think there would ever be a use case where the floating component shouldn't get mirrored based on the writing direction? I can't think of an example right now, but I was wondering if consumers would want to opt out of this middleware (we can also add an option to support that later on if needed)
There was a problem hiding this comment.
I think (and have seen) that most of the cases use either top or bottom (which are not affected by this custom logic), so we should be fine keeping this middleware by default. Also checked in other RTL-friendly websites and have seen that tooltips are mirrored under similar conditions (e.g. booking.com). We can add the option to support that if it is needed :)
There was a problem hiding this comment.
While testing the Default story, I found that the args in the query params weren't working when I'd refresh. I think it's unrelated to the RTL work and is only a storybook thing, but thought I'd mention it!
Screen.Recording.2025-12-01.at.3.47.34.PM.mov
There was a problem hiding this comment.
Good finding! I've fixed it :)
|
|
||
| type Placement = PropsFor<typeof Floating>["placement"]; | ||
|
|
||
| export const Placements: StoryComponentType = { |
There was a problem hiding this comment.
Should we add documentation somewhere so consumers know that the floating direction will be handled for them? Maybe for this story, or in the props docs for the placement prop!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/floating-ui #2881 +/- ##
===========================================
===========================================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary:
Adds right-to-left support to Floating component.
To support this, I've created a custom middleware to be able to mirror the
floating placement in RTL contexts.
Issue: https://khanacademy.atlassian.net/browse/WB-2162
Test plan:
Navigate to any floating stories, select the
RTLmode, and verify that thefloating element flips when the
placementisleft*orright*.