fix: popover alignment for rtl / useLogicalProperties#3326
fix: popover alignment for rtl / useLogicalProperties#3326dreamwasp wants to merge 4 commits intocass-gmt-1601from
Conversation
|
View your CI Pipeline Execution ↗ for commit 5b1c675 ☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## cass-gmt-1601 #3326 +/- ##
================================================
Coverage ? 89.26%
================================================
Files ? 251
Lines ? 4722
Branches ? 1593
================================================
Hits ? 4215
Misses ? 499
Partials ? 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LinKCoding
left a comment
There was a problem hiding this comment.
I'm hovering on approving, but had some questions
The biggest one that gives me pause is how the horizontal alignment example's beaks seem to be inverted when the FlexBox has a set dir.
| paddingInlineStart: containerOffsetVertical, | ||
| insetInlineStart: '100%', |
There was a problem hiding this comment.
Should this and the other alignment styles be conditionally set based on useLogicalProperties?
There was a problem hiding this comment.
good point, will change!
There was a problem hiding this comment.
coming back to this after spending more time on it, and it seems like there is a non-trivial amount of work needed to perform this swapping for physical properties that render correctly for RTL (and LTR)
we can't just use a hook in this, and since this function is being used to generate variants (and not a component), I think the most gamut way to implement this is to create a function that gets the value of useLogicalProperties that then returns the correct CSS for popover alignment, and then some logic for the beak alignment.
But in the process of doing this, I don't know if that's worth the effort (that might need to be refactored later anyway).
I'm going to rescind my comment before, and say that this works well and renders as intended. (After the horizontalBeakRtlInline is added back)
|
|
||
| export const HorizontalAlignmentsRtl: Story = { | ||
| render: () => ( | ||
| <FlexBox dir="rtl" justifyContent="space-around" m={24} width="95%"> |
There was a problem hiding this comment.
QQ: was this just for testing purposes? i.e. cleaned up after? or should it also on the mdx?
I think it's a good example for showcasing floating tooltips :)
There was a problem hiding this comment.
yeah weird, i'll look into it
There was a problem hiding this comment.
i think this was happening cos i (robot) accidentally refactored the physical properties to logical, i'm reverting that! i was just using it for testing but i'll put it int the proper mdx
There was a problem hiding this comment.
I think the horizontalCenterBeakRtlInline function that was removed before works well here.
Curious why it was removed?
I added it back in #3330
and it seems to do the trick
* update story and mdx * restore RTL beaks * add horizontal alignments story * ToolTip text update * applied Amy's feedback
* addressing corner beaks for popover alignment and tests
|
📬 Published Alpha Packages:
|
|
🚀 Styleguide deploy preview ready! Preview URL: https://69eb886c56bb0452e221f539--gamut-preview.netlify.app |


Overview
fixes Popover alignment when
dirisrtl.PR Checklist
Testing Instructions
Don't make me tap the sign.
PR Links and Envs