Feature/allow zero expanded #160
Conversation
There was a problem hiding this comment.
Thanks for the PR @catepalmer. I think in this case you might have misunderstood the desired functionality. You've focused on item removal (AccordionContainer@removeItem), whereas I think you ought to be focussed on AccordionContainer.setExpanded.
Ultimately, the desired outcome looks something like this:
User clicks the only expanded AccordionItem - is collapse permitted?
| Conditions | Collapse Permitted |
|---|---|
allowZeroExpanded={false} (default) |
No |
allowZeroExpanded={true} |
Yes |
EDITED: Table above originally had "Collapse permitted" round the wrong way, as mentioned in subsequent conversation.
| @@ -23,7 +23,7 @@ describe('Accordion', () => { | |||
|
|
|||
| function mountAccordion(): ReactWrapper { | |||
| return mount( | |||
| <Accordion allowMultipleExpanded={false}> | |||
There was a problem hiding this comment.
Nice - that was redundant wasn't it
| const { allowMultipleExpanded, onChange, ...rest } = this.props; | ||
| const { | ||
| allowMultipleExpanded, | ||
| allowZeroExpanded, |
There was a problem hiding this comment.
Good catch. Would have caused console errors otherwise :)
| ); | ||
| }); | ||
|
|
||
| it("setting the expanded property to false in an accordion that doesn't allow zero expansions when there is only one item expanded doesn't close that item", () => { |
There was a problem hiding this comment.
These tests raise some questions about whether or not we should support 'controlled' accordions (ie. the expanded prop). We basically have two conflicting mechanisms for state management, and one can be leveraged/abused to make the accordion not-accessible. Nothing to action here, just a mental note about reviewing the expanded prop functionality (will raise an issue/proposal).
Ah, whoops. I've changed it now so that the new functionality is in AccordionContainer.setExpanded rather than AccordionContainer.removeItem. Hopefully it'll be more on the right track when I push the changes up this time.
Hmm, just to check here - my understanding was that when |
@catepalmer yes you got it! Sorry, I'll update my comment above to reflect this. |
| ...item, | ||
| expanded, | ||
| }; | ||
| if ( |
There was a problem hiding this comment.
Two notes on this little block:
- It could be moved above the
this.setStateblock and just doreturn;to bail out. That way, if the conditions are met thensetStateis never called at all, and an unnecessary re-render is prevented. - The
.length < 2is slightly ambiguous. I think more correctly it would be.length === 1, because this logic should not apply iflength = 0or iflength = 1.5, for example. Both of those examples would not eventuate in practice (particularly1.5haha) but still doesn't hurt to be explicit.
There was a problem hiding this comment.
Ah yep, good thinking - just made those changes 👍
| children?: React.ReactNode; | ||
| items?: Item[]; | ||
| onChange?(args: UUID[]): void; | ||
| }; | ||
|
|
||
| export type AccordionContainer = { | ||
| allowMultipleExpanded: boolean; | ||
| allowZeroExpanded?: boolean; |
There was a problem hiding this comment.
I don't think that this is an optional (?) property of this type. It is optional as a prop though (ProviderProps), just not here.
…cordionContainer type
I've added an
allowZeroExpandedprop (as outlined in issue #153) that determines whether a 'close' action should be blocked if the item is currently the only one open. By default this prop is set tofalse, but it can be set totruein order to enable this functionality. I've also updated and added tests to cover this feature.