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 disabled prop to Accordion.Item. #310

Conversation

@RobertBolender
Copy link
Collaborator

RobertBolender commented Jan 8, 2020

No description provided.

@RobertBolender RobertBolender requested a review from bryanrsmith Jan 8, 2020
);

return (
<Box
display="grid"
css={`
opacity: ${isDisabled ? 0.5 : 1};

This comment has been minimized.

Copy link
@bryanrsmith

bryanrsmith Jan 8, 2020

Collaborator

Is this meant to disable the panel contents as well? If so, we need to disable interaction in the whole subtree as well. e.g., Buttons in the panel should act as standard :disabled buttons.

This comment has been minimized.

Copy link
@RobertBolender

RobertBolender Jan 8, 2020

Author Collaborator

I added a note to the documentation, indicating that it would be the responsibility of the consumer to disable any panel contents or custom header indicators.

https://github.com/Faithlife/styled-ui/pull/310/files#diff-0e61a7ed60261280ca73fe9f59d8bc95R148-R149

In most cases though, we expect that a disabled item will be closed.

This comment has been minimized.

Copy link
@bryanrsmith

bryanrsmith Jan 8, 2020

Collaborator

I'm not sure I see the value in this if we're only handling the visual state, which (I believe) a consumer can already handle on their own trivially, without solving the (far more difficult) behavioral issue.

This comment has been minimized.

Copy link
@RobertBolender

RobertBolender Jan 8, 2020

Author Collaborator

This does solve 90-100% of the behavioral issue though, as it disables the interaction with the header.
We don't have direct control of the panel contents or the custom indicator, so those are still the consumer's responsibility.

This comment has been minimized.

Copy link
@bryanrsmith

bryanrsmith Jan 8, 2020

Collaborator

Disagree. disabled has a very specific meaning, and this prop doesn't conform to what I believe would be a common expectation of it. If it doesn't disable interaction with all the UI it applies to then it's very likely to be a source of bugs. Developers will assume it behaves as other disabled props/attributes do, and fail to truly disable interaction within the panel. Then users will interact with those controls and put components into a state the developer believed to be impossible.

This comment has been minimized.

Copy link
@RobertBolender

RobertBolender Jan 8, 2020

Author Collaborator

I personally would have been surprised if passing a disabled prop into a parent element interfered with the behavior of a child element that I pass into it, but I guess I could understand that another developer may have that expectation.

Here are a few alternative suggestions:

  1. Add the disabled prop to the Accordion.Header instead of the <AccordionItem>. This will allow us to remove the cursor: pointer and the click/keydown handlers from the header. The custom indicator will still be the responsibility of the consumer. We could pass an isDisabled argument to the custom indicator render prop.

  2. Require the consumer to handle disabled behavior within the onExpansion callback. Before setting the section as open, first check whether or not conditions exist for which the section should be disabled and therefore not expanded.

This comment has been minimized.

Copy link
@bryanrsmith

bryanrsmith Jan 8, 2020

Collaborator

Does option 1 differ in behavior from pinned? I don't understand option 2.

  1. Add a new component capable of disabling a subtree. It could either use React context to communicate state to other styled-ui form components (and not work with non-styled-ui components), or it could use a more heavy-handed approach like what https://github.com/GoogleChrome/inert-polyfill does.
  2. Don't handle it in styled-ui, or defer until a different project needs it.

I'm in favor of option 4.

This comment has been minimized.

Copy link
@RobertBolender

RobertBolender Jan 8, 2020

Author Collaborator

Option 1 would have the same behavior as pinned, so we can use that. I had mistakenly thought that pinned could only be used with the open state, but that is not the case. There was a minor style difference between pinned and disabled, but I'll ask Shiloh if we can ignore that for now.

Option 2 was describing my thinking about a workaround for implementing this functionality without this PR, but if we can just use pinned then it will be unnecessary.

I think we can move forward without this PR, using pinned and passing in an opacity prop to the Accordion.Item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.