-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: allow customizing behavior of pressed state #8971
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
base: main
Are you sure you want to change the base?
Conversation
|
Build of PR over here: #9065 for ease of testing |
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.
Thanks so much for starting this PR! and thank you so much for writing tests!
| formValue = 'key', | ||
| allowsCustomValue | ||
| allowsCustomValue, | ||
| isTriggerPressedWhenOpen = true |
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.
generally we try to have booleans be negative by default so that you don't need to explicitly set false on it and omitting it is the same as setting false
this is a really hard one to name
isTriggerUpWhenOpen
isTriggerInactiveWhenOpen
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.
I like isTriggerUpWhenOpen, or maybe as alternative suggestion isTriggerReleasedWhenOpen (but it's a bit long). But it's up to you to decide.
|
found a small issue, just a missed component, while I was testing this with our visual regression. I've fixed it. I also managed to dig up our current majority opinion. We like |
Closes #8339
Several components like Dialog and Select set
data-pressed="true"on the trigger while the corresponding overlay is open, which is often not desired for styling. As outlined in the issue, a new property has been added to these components to prevent this behavior. The default value istrue, so the behavior remains unchanged by default.Naming: A few names for the property were suggested in the issue. I used
isPressedWhenOpenon trigger components (DialogTrigger, MenuTrigger), andisTriggerPressedWhenOpenfor other components to make it clear that the property affects the trigger.I think it's easier to understand than
consideredPressedWhenOpen. AndpersistPressStateWhenOpendoesn't feel right for me for components likeMenuwhich open the overlay on press start and therefore never havedata-pressed=truewith the new behavior.Storybook: I added a new control to the corresponding RAC stories which already used Story controls. Most of them don't and I didn't add a separate Story for it. If I should add them, let me know.
Spectrum: I also updated the S2 components, I hope that's okay.
DialogTriggerPropsto fully extend the RACDialogTriggerProps(which was the case before). Is that ok?isPressed={false}) are removed.TabsPickercomponent.✅ Pull Request Checklist:
📝 Test Instructions:
yarn startdata-pressed="true"isTriggerPressedWhenOpentofalsedata-pressedis not set anymore🧢 Your Project: