-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Feat: Add support for horizontal orientation
to GridList
& ListBox
#8533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…to layout-orientation
…ayout-orientation
@LFDanLu Would you be so kind to issue a build for this PR? I'm going insane because of a bug with virtualized dnd, which I can't seem to get rid of locally, even after reverting all changes and clearing caches. I would really like to know whether the issue exists on remote 😅 |
|
||
constructor(ref: RefObject<HTMLElement | null>) { | ||
constructor(ref: RefObject<HTMLElement | null>, orientation?: Orientation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely happy with having to pass this here, but I couldn't think of any reliable alternative. I guess we could do something similar to the drop target delegate and place this information in a data attribute, but is this really preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd rather not rely on a data attribute for this, this is fine for now IMO. Having the aria roles and their orientations here in the delegate is interesting, wonder if it would make sense to instead only rely on the higher level delegate (and by extension the hook that creates the keyboard delegate) to pass the appropriate orientation. I guess this makes for a good fallback as is though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted with the team some more about this, and we wondered if we really need have the orientation information here in the DOMLayoutDelegate since this delegate is mainly meant to inform about stuff like itemRects and sizes. What was the need for this information to be placed here since the keyboard delegates already were passed orientation via their options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking about why the LayoutDelegate
interface in general requires an orientation or rather about DOMLayoutDelegate
specifically? If the later, then as you speculated it's just meant to provide reasonable fallback in case the keyboard delegate is not passed an orientation - can remove if you prefer.
In case you are wondering why we expose a getOrientation()
method instead of keeping this option protected in the layout, its because we can utilize this method to provide a fallback in the keyboard delegates and therefore make the orientation
prop on the hook optional. This way a user doesn't have to set orientation=horizontal
both in the layout options and on the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mainly was concerned about the DOMLayoutDelegate
, but was wondering if a fallback was really needed. I was imagining that the orientation would be passed from the component level if customizable or have its default set by the respective hook and passed to the delegate, though I do see your point about then needing the user to provide it to the Virtualizer's layout as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I felt like it would be fairly easy for a user to miss the orientation
on the component and just set it in the layout since it will already look right, just wont be working correctly for keyboard users. Having it that way ensures we are in sync, especially with the discussed warning message.
orientation
to ListLayout
delegateorientation
to GridList
& ListBox
@@ -380,6 +381,7 @@ export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxSta | |||
shouldUseVirtualFocus: true, | |||
shouldSelectOnPressUp: true, | |||
shouldFocusOnHover: true, | |||
orientation: 'vertical' as const, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should explicitly set orientation
whenever known to avoid queries to the DOMLayoutDelegate
internally.
@@ -117,6 +123,7 @@ export function useListBox<T>(props: AriaListBoxOptions<T>, state: ListState<T>, | |||
'aria-multiselectable': 'true' | |||
} : {}, { | |||
role: 'listbox', | |||
'aria-orientation': orientation === 'horizontal' ? orientation : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@majornista Should we set aria-orientation
only if deviating from the implicit orientation as defined in each role spec? I adjusted useTabList
as well to match, let me know what you think 🙏
@nwidynski heya sorry about the delay, opened #8544 for your build |
@LFDanLu Thanks! I will take a look tmrw, otherwise this is ready for review if the team finds time 👍 |
sounds good, we are gearing up for a release very soon, but noted this one down (and #8523) for the team to checkout right after! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be bring this up with the team tomorrow, but looks good to me at a glance, thanks!
|
||
constructor(ref: RefObject<HTMLElement | null>) { | ||
constructor(ref: RefObject<HTMLElement | null>, orientation?: Orientation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd rather not rely on a data attribute for this, this is fine for now IMO. Having the aria roles and their orientations here in the delegate is interesting, wonder if it would make sense to instead only rely on the higher level delegate (and by extension the hook that creates the keyboard delegate) to pass the appropriate orientation. I guess this makes for a good fallback as is though
// TODO: Should we log a warning if keyboard and layout delegate mismatch in orientation? | ||
return this.orientation || this.layoutDelegate.getOrientation?.() || 'vertical'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to log a dev mode only warning here, save some pain down the line haha
|
||
constructor(ref: RefObject<HTMLElement | null>) { | ||
constructor(ref: RefObject<HTMLElement | null>, orientation?: Orientation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted with the team some more about this, and we wondered if we really need have the orientation information here in the DOMLayoutDelegate since this delegate is mainly meant to inform about stuff like itemRects and sizes. What was the need for this information to be placed here since the keyboard delegates already were passed orientation via their options?
mainly wondering if some of the changes/refactors can be simplified since we already supported orientation for Listbox previously |
@LFDanLu Maybe the PR title is misleading - The primary target of this is to add |
b6452fc
to
2d97559
Compare
This PR adds support for
horizontal
orientations toListLayout
and affected components in preparation for aCarousel
contribution.✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: