fix(accordion-group): skip initial animation #30729
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@ShaneK Thanks for fixing this! It looks great in the main react use examples, but I did find the following case where it doesn't animate initially. It used to work before but maybe I'm not doing things right I just wanted to let you know :P https://stackblitz.com/edit/dbaxphvw-jt3durvs?file=src%2Fmain.tsx |
Excellent catch, new dev build should address this. |
|
Thanks works great! |
| const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; | ||
|
|
||
| expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); | ||
| expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); |
There was a problem hiding this comment.
I didn't realize that spec tests capture these kinds of classes. I would have expected it to be false all the time since expect would run after the animation ended. I'll have to keep this in mind for future tests. Thanks!
Okay, I think I've got it! New dev build: |
There was a problem hiding this comment.
I found an issue with expanded accordions on the basic test. They do not animate on collapse:
accordion-branch.mov
…kip state and allow animations on first interaction
New dev build: |
… forceUpdate to try to prevent race conditions
There was a problem hiding this comment.
Can we revert the changes to this file since it doesn't do anything?
| const value = accordionGroup.value; | ||
| const accordionValue = this.value; | ||
| const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; | ||
| const isExpanded = this.state === AccordionState.Expanded || this.state === AccordionState.Expanding; | ||
| const stateWillChange = shouldExpand !== isExpanded; |
There was a problem hiding this comment.
Nit: It might make more sense to clarify which value belongs to the group, since I'd expect value to refer to the current component:
| const value = accordionGroup.value; | |
| const accordionValue = this.value; | |
| const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; | |
| const isExpanded = this.state === AccordionState.Expanded || this.state === AccordionState.Expanding; | |
| const stateWillChange = shouldExpand !== isExpanded; | |
| const groupValue = accordionGroup.value; | |
| const value = this.value; | |
| const shouldExpand = Array.isArray(groupValue) ? groupValue.includes(value) : groupValue === value; | |
| const isExpanded = this.state === AccordionState.Expanded || this.state === AccordionState.Expanding; | |
| const stateWillChange = shouldExpand !== isExpanded; |
| * This prevents the initial undefined value from the group's componentDidLoad | ||
| * from being treated as the first real update. | ||
| */ | ||
| if (value !== undefined) { |
There was a problem hiding this comment.
| if (value !== undefined) { | |
| if (groupValue !== undefined) { |
See above
| export class Accordion implements ComponentInterface { | ||
| private accordionGroupEl?: HTMLIonAccordionGroupElement | null; | ||
| private updateListener = () => this.updateState(false); | ||
| private updateListener = () => { |
There was a problem hiding this comment.
It would be great to take this opportunity to change the name to something a bit more descriptive. I'll leave it up to you though if the change needs to be done.
Issue number: resolves #30613
What is the current behavior?
Currently, when you load an accordion group, the initially selected accordion animates open. This is an unexpected change caused by upgrading Stencil to >= 4.21.0, which changed the way component lifecycles got triggered.
What is the new behavior?
With this change, we're waiting for the accordion in the accordion group to render and telling it if the update it's going through is the initial update or not. This allows it to decide to animate properly.
Does this introduce a breaking change?
Other information
Current dev build: