[DropdownMenu][ContextMenu][RadioGroup][ScrollArea][Slider][Tabs][ToggleGroup][Toolbar] Direction provider #1119
Conversation
dba1ae5 to
d928cd6
Compare
| @@ -125,7 +130,6 @@ const TabsList = React.forwardRef<TabsListElement, TabsListProps>( | |||
| <Primitive.div | |||
| role="tablist" | |||
| aria-orientation={context.orientation} | |||
| dir={context.dir} | |||
There was a problem hiding this comment.
Moved this to Tabs.Root div instead so that right-to-left content works.
| /** | ||
| * The direction of navigation between menu items. | ||
| * @defaultValue ltr | ||
| */ | ||
| dir?: RovingFocusGroupProps['dir']; | ||
|
|
There was a problem hiding this comment.
I realised this was accepting dir prop but RovingFocusGroup never used it because it gets it from root context instead.
2b47484 to
773a005
Compare
|
Looking at chromatic, it looks like the scrollarea one is broken in the inherited scenario? |
I thought that too but it's not, it's because the direction is added to the scrollarea div now from context (which is fixed width) instead of being added to a 100% width div that wraps it. We inherit from context now instead of from a parent element. The important part is that the vertical scroll is on left and thumb starts from right in horizontal bar. |
There was a problem hiding this comment.
It seems a bit all over in terms of conventions with regards to the dir prop's default value.
It looks like you intentionally removed the default value in destructuring in places, but then sometimes a default is added inline after the useDirection() hook, sometimes not, and other times a default is added inline for specific use-cases.
There's probably a reason behind all that, but after reading this I'm wondering why we can't always default to ltr in destructuring, thus insuring there will always be a value no matter what and the convention is the same everywhere.
As to the versioning I'm still unclear what we should do here, probably ok for now to go with patches…
f23bdb5 to
ca8c652
Compare
|
|
||
| function useDirection(localDir?: Direction) { | ||
| const globalDir = React.useContext(DirectionContext); | ||
| return localDir || globalDir || 'ltr'; |
There was a problem hiding this comment.
I moved the fallback logic into the hook as I found it clearer to see the cascade preference.
@benoitgrelard re your last round of comments – I couldn't see any reason not to provide the default ltr value so I resolved them and went for this, let me know what you think though.
Edit – I think chromatic has just shown me the reason 😅
There was a problem hiding this comment.
That's funny cause Jenna had done that originally which I asked about: #1119 (comment)
@benoitgrelard re your last round of comments – I couldn't see any reason not to provide the default ltr value so I resolved them and went for this, let me know what you think though.
Edit – I think chromatic has just shown me the reason 😅
Can you provide more detail about this bit, i'm not following.
There was a problem hiding this comment.
We checked in offline about this, the primary reason that I went back to passing an arg to the hook is for style. Because we can’t conditionally call hooks it’d be more verbose when done inline:
const contextDir = useDirection();
const direction = contextDir || dirProp || 'ltr' ;vs
const direction = useDirection(dirProp)The latter of which we seem to prefer on balance.
Can you provide more detail about this bit, i'm not following.
Seems that the original changes in this PR removed default values in certain places due to how some primitives get recomposed and the cascading effect of always setting a dir attribute from a default value. This is made clear by a preexisting bug in Toolbar where ToolbarToggleGroup doesn't respect dir="rtl" when added to Root, I think the changes were to remedy these issues.
We decided it would be clearer overall to provide a default in all cases and ensure we consistently pass dir values down the tree where necessary.
ca8c652 to
70a00ff
Compare
| @@ -177,6 +176,7 @@ const ToolbarToggleGroup = React.forwardRef< | |||
| return ( | |||
| <ToggleGroupPrimitive.Root | |||
| data-orientation={context.orientation} | |||
| dir={context.dir} | |||
There was a problem hiding this comment.
We need to pass this down to ToggleGroupPrimitive as we now consistently provide a fallback dir value. Without doing so ToolbarToggleGroup renders dir="ltr" if DirectionProvider is not used and <Toolbar.Root dir="rtl"/>, thus we pass it explicitly via context to prevent that.
…gleGroup][Toolbar] Direction provider (radix-ui#1119) * Remove useDirection raf * Use direction context in other comps * Remove useDirection param * Update `useDirection` usage Co-authored-by: Andy Hook <[email protected]>
Closes #749
react-use-directionpackage toreact-directionand exposesDirectionProviderShould I bump these as a minor?
Notes for docs
directionpackage withDirectionProviderto change Radix components direction at app level. This used to inherit so folks might have breaking changes.dirattribute. It'sundefinednow.dirprop fromContextMenu.ContenttoRootpart to match implementation