feat(accordion): add methods to expand/collapse/collapseAll panels #1978
Conversation
|
What needs to be done to get this into a build? Thanks. |
|
Hopefully this is accepted. Can you add expandAll() as well? |
|
@christarczon I don't know if I have missed a step or why this has not been accepted. I wish someone would please advise, I would like to use this from an official release. An expandAll could be added easily. |
| const panel = this.panels.find(p => p.id === panelId); | ||
|
|
||
| if (panel && !panel.disabled && !panel.isOpen) { | ||
| this.toggle(panelId); |
There was a problem hiding this comment.
IMO we should it differently - the logic of expanding should be in this method and toggle should call it based on the state. In other words - the toggle method should make use of the new expand / collapse and not the other way around.
| */ | ||
| isExpanded(panelId): boolean { | ||
| const panel = this.panels.find(p => p.id === panelId); | ||
| return panel.isOpen; |
There was a problem hiding this comment.
This will crash if there is no panel with a given panelId if I'm not mistaken? Could you please add a test for this case and fix the code if needed?
| * Programmatically collapse all panels. | ||
| */ | ||
| collapseAll() { | ||
| this.panels.forEach(panel => { panel.isOpen = false; }); |
There was a problem hiding this comment.
This should rather delegate to the collapse call - otherwise we are not taking the disabled flag into account. Please add a test for the case assuring that we are not collapsing an open, disabled panel.
There was a problem hiding this comment.
I wasn't sure if the disabled flag should take effect here or not, because the 'closeOtherPanels' option I believe takes effect even when a panel is disabled (see toggle and _closeOthers).
There was a problem hiding this comment.
Also, I am not sure how to make a panel in the state of 'open and disabled'. Is it possible? If I set disabled on a panel, it closes. How do I do this?
| /** | ||
| * Programmatically collapse all panels. | ||
| */ | ||
| collapseAll() { |
There was a problem hiding this comment.
We should also add expandAll (+ tests) for symmetry.
|
@Thorski sorry for the delay reviewing this PR - I've finally got to it and left few comments. Generally speaking:
Additionally I would like to propose that we do the following renaming:
This would align names with other directives in this library. WDYT? |
|
This PR doesn't merge cleanly, is very light on tests and has several review comments not addressed so we are going to work towards merging #2595 instead. |
feat(accordion): add methods to expand/collapse/collapseAll panels
Added
expand, collapse, collapseAll, and isExpanded methods.
#1970