Skip to content

Conversation

@YumiChen
Copy link
Contributor

@YumiChen YumiChen commented Mar 13, 2025

Closes #7887

  • Add shouldSelectOnPressUp prop to GridList following ListBox
  • Add shouldSelectOnPressUp to GridList story

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@YumiChen YumiChen closed this Mar 13, 2025
@YumiChen YumiChen reopened this Mar 13, 2025
@YumiChen YumiChen force-pushed the 7887-add-shouldSelectOnPressUp-prop-to-GridList branch from d4d3d1e to b47c9f9 Compare March 13, 2025 04:39
@YumiChen YumiChen marked this pull request as ready for review March 13, 2025 04:51
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, would you mind adding tests in react-aria-components/test/GridList.test.js ?
I'd point you to ListBox's tests, but it seems it's tested implicitly by Menu, but it looks like there's no coverage for GridList.

If you don't have time, let us know.
Thank you!

@YumiChen
Copy link
Contributor Author

@snowystinger Thank you for the reminder. I added some unit tests and verified the changes by following the test instructions included in the PR description. Please let me know if there are any suggestions

@YumiChen YumiChen requested a review from snowystinger March 15, 2025 07:36
snowystinger
snowystinger previously approved these changes Mar 17, 2025
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much! I added a story specifically for the use case to make sure that was resolved. It looks good!

@YumiChen
Copy link
Contributor Author

Got it. Thank you @snowystinger!

snowystinger
snowystinger previously approved these changes Apr 9, 2025
@devongovett
Copy link
Member

Doesn't look like this is currently a public prop in ListBox, only an option of the useListBox hook. Do we want to expose it on ListBox too?

@devongovett devongovett force-pushed the 7887-add-shouldSelectOnPressUp-prop-to-GridList branch from b43053e to 6db09e7 Compare April 11, 2025 22:13
@devongovett devongovett changed the title Feat: add shouldSelectOnPressUp prop to GridList Feat: add shouldSelectOnPressUp prop to collection components Apr 11, 2025
devongovett
devongovett previously approved these changes Apr 11, 2025
LFDanLu
LFDanLu previously approved these changes Apr 11, 2025
…shouldSelectOnPressUp-prop-to-GridList

# Conflicts:
#	packages/react-aria-components/stories/GridList.stories.tsx
#	packages/react-aria-components/test/TagGroup.test.js
@LFDanLu LFDanLu added this pull request to the merge queue Apr 12, 2025
Merged via the queue into adobe:main with commit c73bebd Apr 12, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GridList (in Popover) closes containing Modal on select

4 participants