feat(Accordion): ability to open multiple items #976
Conversation
Allowing multiple open panels at once, need help with transforming activeIndex prop to an array.
| activeIndex: PropTypes.number, | ||
| activeIndex: PropTypes.oneOfType([ | ||
| PropTypes.number, | ||
| PropTypes.array, |
There was a problem hiding this comment.
Let's beef this up a bit, PropTypes.arrayOf(PropTypes.number).
| @@ -38,6 +45,9 @@ export default class Accordion extends Component { | |||
| /** Initial activeIndex value. */ | |||
| defaultActiveIndex: PropTypes.number, | |||
|
|
|||
| /** Allow only one index */ | |||
| exclusive: PropTypes.bool, | |||
There was a problem hiding this comment.
We try to stick very close to SUI core descriptions. Here's the description from the Accordion settings page:
/** Only allow one section open at a time */Since we currently have a deviation in terms, panels vs sections, let's keep consistent with panels. Suggest we go with:
/** Only allow one panel open at a time */
|
I have left a couple minor comments for now. If you can describe the issue you are having in full detail, I can take a look and offer some help. Thanks for the PR! |
| //TODO: Can not accept an activeIndex and be inclusive at the same time | ||
| // if(this.props.exclusive == false && _.isNumber(this.props.activeIndex)){ | ||
| // this.trySetState({ activeIndex: [ this.props.activeIndex ] }) | ||
| // } |
There was a problem hiding this comment.
@levithomason My Issue is when exclusive={false} and activeIndex={1}, I can't transform the activeIndex to an array.
I tried this but to no avail. i'm not sure how the internals are working, but i think the props is overwriting my state change.
There was a problem hiding this comment.
Correct, this is an AutoControlledComponent which means, if the user passes a prop then they are in control of the state but when there is no prop the Component is in control of the state.
this.trySetState will only set state if there is no prop defined. In this regard, the component "auto controls" state for the user but also backs off if the user controls it with props.
It is up to the user to pass the correct combination of activeIndex and exclusive. The alternative is to pass defaultActiveIndex which is only applied on the first render, then the component can update theactiveIndex on subsequent renders.
There was a problem hiding this comment.
So if you I understand correctly defaultActiveIndex should be allowed to a number or array from props? And if defaultActiveIndex={0} and exclusive={false} are set add it to the activeIndex array, i.e. activeIndex=[0].
There was a problem hiding this comment.
Yes, except, most of that happens automatically. The ACC (AutoControlledComponent) handles copying props and default props to their regular prop name on state.
Current coverage is 99.78% (diff: 80.00%)@@ master #976 diff @@
========================================
Files 137 137
Lines 2284 2303 +19
Methods 0 0
Messages 0 0
Branches 0 0
========================================
+ Hits 2284 2298 +14
- Misses 0 5 +5
Partials 0 0
|
|
@levithomason let me know if i got anything wrong |
|
Sounds good, I've marked this for review and will get to it soon as I am able. Thanks! |
|
I made some cleanup and minor fixes to this PR, however, I cannot push them since it is pointing to your master branch. I've opened #988 branching off of your fork and including my updates. I'll close this PR and merge the other. Let's move all future convo over there 👍 |
Add ability for activeIndex in Accordions to be an array.
Allowing multiple open panels at once, need help with transforming
activeIndex prop to an array.