Open ArrayKnight opened 1 week ago
Thanks for the issue, we're aware of the inconsistency and have been talking about ways to fix it without it being a breaking change.
Yea, it's a tricky corner you're in. Seems like there's only a few options, all with their drawbacks:
#1
isOpen
render prop & data attribute to Button that carries this state through from props. This doesn't make a whole lot of sense since the Button shouldn't really understand the concept of open since it has no directly associated state. You'd end up likely pairing this solution with #2
anyways for overall consistencyThat could look like:
// This wouldn't be a breaking change, but it would be a bit of
// a type nightmare in terms of an implementor not knowing
// which values are actually being fulfilled and by which parent
type FlatRenderProps = Partial<ButtonRenderProps> & Partial<SelectRenderProps> & ... // so on for all components
// This would be a breaking change since all render props
// are currently top level properties. But it has the advantage
// of making sure properties are clearly delineated
type NestedRenderProps = {
button?: ButtonRenderProps;
select?: SelectRenderProps;
... // so on for all components
}
Provide a general summary of the issue here
A bit of an undocumented feature that doesn't seem to work completely as intended. Select and ComboBox both keep the button in the pressed state if the popover is in an open state:
https://github.com/adobe/react-spectrum/blob/main/packages/react-aria-components/src/Select.tsx#L170
https://github.com/adobe/react-spectrum/blob/main/packages/react-aria-components/src/ComboBox.tsx#L185
In the render props, this doesn't work and the isOpen render prop from the Select / ComboBox must be used
However, the data attributes on the Button do reflect the persisted pressed state. This inconsistency in state representation is undesirable
π€ Expected Behavior?
It would be nice to isolate render functions as much as possible and not have them wrapped around so much of the children components. So, it would be desirable to be able to use the isPressed render prop from the Button to change icons based on state
π― Current Behavior
IsPressed render prop from Button corresponds to user interaction and is not overridden by state provided by Select/Combobox as seems to be intended
π Possible Solution
No response
π¦ Context
No response
π₯οΈ Steps to Reproduce
https://codesandbox.io/p/sandbox/musing-maxwell-y3p2hy?workspaceId=9ca74201-e2f5-4f45-ad8e-200e6382b9bd
Version
RAC @ 1.3.1
What browsers are you seeing the problem on?
Chrome
If other, please specify.
No response
What operating system are you using?
Mac & Windows
π§’ Your Company/Team
No response
π· Tracking Issue
No response