Skip to content
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

Add renderProp support for if a Table row has visible focus within it #6350

Merged
merged 6 commits into from
May 28, 2024

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented May 8, 2024

Closes #5258

useful for making a row style that remains applied when anything in the table row is focused, not just the row itself (e.g. make the whole row blue when a cell inside is keyboard focused). Needed for S2.

Other changes to come as I run into them while working on S2

✅ 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:

RSP

useful for making a row style that remains applied when anything in the table row is focused, not just the row itself
@rspbot
Copy link

rspbot commented May 8, 2024

@rspbot
Copy link

rspbot commented May 14, 2024

@rspbot
Copy link

rspbot commented May 15, 2024

@jaknas
Copy link

jaknas commented May 17, 2024

Can be linked to: #5258 (my initial issue was to support it across whole RAC, but only needed it in the Row, which this PR seems to add 🙏 )

@LFDanLu
Copy link
Member Author

LFDanLu commented May 22, 2024

@jaknas thanks for the issue link (may have slipped my mind to see if anyone filed a issue for this change when making the PR 😅). I'm still evaluating what other elements should also expose some version of isFocusVisibleWithin as I work on styling an adjacent component that uses RAC table but can definitely get this merged in for now so a nightly can built. Do you also need info for isFocusWithin or is isFocusVisibleWithin sufficient for your use case?

this is for s2, make keyboard skip the resizer via data-react-aria-prevent-focus
@rspbot
Copy link

rspbot commented May 22, 2024

@jaknas
Copy link

jaknas commented May 28, 2024

@jaknas thanks for the issue link (may have slipped my mind to see if anyone filed a issue for this change when making the PR 😅). I'm still evaluating what other elements should also expose some version of isFocusVisibleWithin as I work on styling an adjacent component that uses RAC table but can definitely get this merged in for now so a nightly can built. Do you also need info for isFocusWithin or is isFocusVisibleWithin sufficient for your use case?

I can't say for sure without testing. isFocusVisibleWithin should be enough for starters.

@LFDanLu
Copy link
Member Author

LFDanLu commented May 28, 2024

@jaknas gotcha, I'll see about getting this merged in early then so we can get a nightly out for testing.

@LFDanLu LFDanLu marked this pull request as ready for review May 28, 2024 16:41
@rspbot
Copy link

rspbot commented May 28, 2024

@rspbot
Copy link

rspbot commented May 28, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@LFDanLu LFDanLu changed the title (WIP) Add renderProp support for if a Table row has visible focus within it Add renderProp support for if a Table row has visible focus within it May 28, 2024
@LFDanLu LFDanLu merged commit d7fc932 into main May 28, 2024
26 checks passed
@LFDanLu LFDanLu deleted the table_update_for_s2 branch May 28, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for data-focus-within in RAC
6 participants