-
Notifications
You must be signed in to change notification settings - Fork 16
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
docs: Menu and MenuTrigger Docs #1051
Conversation
_table = new_table( | ||
[ | ||
string_col("Keys", ["key-0", "key-1", "key-2"]), | ||
string_col("Labels", ["Option 0", "Option 1", "Option 2"]), | ||
] | ||
) | ||
|
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.
Would be cool to show use with a ticking table 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.
Would we really want to use a ticking table as item source for a menu? I suppose it would work, but I dislike the idea of menu items appearing / disappearing as the table ticks. Also, what would happen if the menu was open while the table ticked? Does it update, or is the menu now out of date?
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.
Guess we should clarify the behaviour with @dsmmcken , but I see a few possible options when the table ticks while the menu is open (ordered by my preference):
- Update the menu in place. Seems this is what Spectrum will do by default: https://codesandbox.io/p/sandbox/react-spectrum-playground-forked-rfctrs
- Dismiss the menu. They need to re-open the menu.
- Freeze the items in the menu while it's open. Could mean some actions are out of date and will error when selected.
We definitely need to decide on what to do with a ticking table... and if you're hooking up a ticking table to a menu, I imagine having it update in place is acceptable.
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.
Spun this off into another ticket:
https://deephaven.atlassian.net/browse/DH-18305
Going to remove this section from the doc for now.
Co-authored-by: Mike Bender <[email protected]>
Jira ticket: https://deephaven.atlassian.net/browse/DH-18089