Skip to content

feat: Allow scroll events to be added on certain table components #8150

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
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lukpsaxo
Copy link
Contributor

Closes #8040
Related (but not closing) #7987

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

N/A

🧢 Your Project:

Saxo Bank

@lukpsaxo
Copy link
Contributor Author

lukpsaxo commented May 6, 2025

@yihuiliao I've no idea who/how to ping here but I'd love a approve or reject or a timeline if you are able.

If the decision is not to support this kind of thing and instead e.g. only support long press when it is added as a real feature, we can plan around that.

@snowystinger
Copy link
Member

We've been a little busy with the latest release, hopefully we'll have a look at this in the near future. Thanks for your patience.

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.

Had a chance to discuss as a team today. We're ok with the onScroll to start, we're not quite ready for onContextMenu at this point because it may interfere with our long press handling on touch. We'd like to think that one over more.

@lukpsaxo
Copy link
Contributor Author

Thanks @snowystinger

We've implemented it in our app and you are right - it conflicts on mobile. Our solution is that we disable drag and drop on mobile and add a mobile "edit mode" that shows the grid in drag and drop mode.

I would be ok with only allowing attaching onContextMenu if there are no drag and drop hooks - maybe thats hard to communicate? but then alot of tables don't have drag and drop.

I'll make the changes suggested in this PR and wait for any decision on context menu.

@lukpsaxo lukpsaxo changed the title feat: Allow scroll and context menu events to be added on certain table components feat: Allow scroll events to be added on certain table components May 23, 2025
@lukpsaxo lukpsaxo force-pushed the ci-table-handlers branch from ff69cf4 to b472822 Compare May 23, 2025 09:50
@lukpsaxo
Copy link
Contributor Author

Updated

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.

Thank you!
In the meantime, if you know the rules around conflicts with Drag and Drop/actions/etc and anything else, then you should be able to use context menus through the ref attached to any given element.

@lukpsaxo
Copy link
Contributor Author

Thanks! We are currently patching react-aria-components as attaching event handlers using refs is a bit messy

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.

[RAC] Allow adding onScroll handlers for table components
2 participants