-
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
Open
nwidynski
wants to merge
10
commits into
adobe:main
Choose a base branch
from
nwidynski:layout-orientation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+311
−166
Open
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
b07954a
feat: add orientation to layout
nwidynski ef61fa4
Merge branch 'main' of https://github.com/nwidynski/react-spectrum in…
nwidynski 36ad8c4
feat: forward orientation to gridlist keyboard delegate
nwidynski bc57b0c
Merge branch 'main' of https://github.com/adobe/react-spectrum into l…
nwidynski e92c7db
fix: remove duplicate keyboard delegate in gridlist
nwidynski 58ad7a2
Feat: Add support for `orientation` to `KeyboardDelegate` interface
nwidynski 22ddea9
fix: implement layout delegate interface
nwidynski 2d97559
fix: enumerable keys
nwidynski ecd054d
Merge branch 'main' of https://github.com/nwidynski/react-spectrum in…
nwidynski d6a8401
fix: bidirectional overscan for table layout
nwidynski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
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?
Uh oh!
There was an error while loading. Please reload this page.
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 aboutDOMLayoutDelegate
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 theorientation
prop on the hook optional. This way a user doesn't have to setorientation=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.