-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(accordion-group): skip initial animation #30729
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
Changes from 10 commits
58c2a8a
761d06f
1a1dcda
2095f2f
63f32a5
cc125e6
90ebfba
e345efd
f84c484
0ce05dc
31bda9d
10eabb3
c1973cc
24a3db0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,40 @@ const enum AccordionState { | |
| }) | ||
| export class Accordion implements ComponentInterface { | ||
| private accordionGroupEl?: HTMLIonAccordionGroupElement | null; | ||
| private updateListener = () => this.updateState(false); | ||
| private updateListener = () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think the current naming is rough, it sorta implies the accordion's updates instead of the group's updates. I tried to improve it a bit, but naming things is hard: 24a3db0 |
||
| /** | ||
| * Determine if this update will cause an actual state change. | ||
| * We only want to mark as "interacted" if the state is changing. | ||
| */ | ||
| const accordionGroup = this.accordionGroupEl; | ||
| if (accordionGroup) { | ||
| const value = accordionGroup.value; | ||
| const accordionValue = this.value; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you undo the rename here? |
||
| const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; | ||
| const isExpanded = this.state === AccordionState.Expanded || this.state === AccordionState.Expanding; | ||
| const stateWillChange = shouldExpand !== isExpanded; | ||
|
ShaneK marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * Only mark as interacted if: | ||
| * 1. This is not the first update we've received with a defined value | ||
| * 2. The state is actually changing (prevents redundant updates from enabling animations) | ||
| */ | ||
| if (this.hasReceivedFirstUpdate && stateWillChange) { | ||
| this.hasInteracted = true; | ||
| } | ||
|
|
||
| /** | ||
| * Only count this as the first update if the group value is defined. | ||
| * This prevents the initial undefined value from the group's componentDidLoad | ||
| * from being treated as the first real update. | ||
| */ | ||
| if (value !== undefined) { | ||
|
ShaneK marked this conversation as resolved.
|
||
| this.hasReceivedFirstUpdate = true; | ||
| } | ||
| } | ||
|
|
||
| this.updateState(); | ||
| }; | ||
| private contentEl: HTMLDivElement | undefined; | ||
| private contentElWrapper: HTMLDivElement | undefined; | ||
| private headerEl: HTMLDivElement | undefined; | ||
|
|
@@ -50,6 +83,25 @@ export class Accordion implements ComponentInterface { | |
| @State() state: AccordionState = AccordionState.Collapsed; | ||
| @State() isNext = false; | ||
| @State() isPrevious = false; | ||
| /** | ||
| * Tracks whether a user-initiated interaction has occurred. | ||
| * Animations are disabled until the first interaction happens. | ||
| * This prevents the accordion from animating when it's programmatically | ||
| * set to an expanded or collapsed state on initial load. | ||
| */ | ||
| @State() hasInteracted = false; | ||
|
|
||
| /** | ||
| * Tracks if this accordion has ever been expanded. | ||
| * Used to prevent the first expansion from animating. | ||
| */ | ||
| private hasEverBeenExpanded = false; | ||
|
|
||
| /** | ||
| * Tracks if this accordion has received its first update from the group. | ||
| * Used to distinguish initial programmatic sets from user interactions. | ||
| */ | ||
| private hasReceivedFirstUpdate = false; | ||
|
|
||
| /** | ||
| * The value of the accordion. Defaults to an autogenerated | ||
|
|
@@ -88,7 +140,7 @@ export class Accordion implements ComponentInterface { | |
| connectedCallback() { | ||
| const accordionGroupEl = (this.accordionGroupEl = this.el?.closest('ion-accordion-group')); | ||
| if (accordionGroupEl) { | ||
| this.updateState(true); | ||
| this.updateState(); | ||
| addEventListener(accordionGroupEl, 'ionValueChange', this.updateListener); | ||
| } | ||
| } | ||
|
|
@@ -212,10 +264,16 @@ export class Accordion implements ComponentInterface { | |
| ionItem.appendChild(iconEl); | ||
| }; | ||
|
|
||
| private expandAccordion = (initialUpdate = false) => { | ||
| private expandAccordion = () => { | ||
| const { contentEl, contentElWrapper } = this; | ||
| if (initialUpdate || contentEl === undefined || contentElWrapper === undefined) { | ||
|
|
||
| /** | ||
| * If the content elements aren't available yet, just set the state. | ||
| * This happens on initial render before the DOM is ready. | ||
| */ | ||
| if (contentEl === undefined || contentElWrapper === undefined) { | ||
| this.state = AccordionState.Expanded; | ||
| this.hasEverBeenExpanded = true; | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -227,6 +285,12 @@ export class Accordion implements ComponentInterface { | |
| cancelAnimationFrame(this.currentRaf); | ||
| } | ||
|
|
||
| /** | ||
| * Mark that this accordion has been expanded at least once. | ||
| * This allows subsequent expansions to animate. | ||
| */ | ||
| this.hasEverBeenExpanded = true; | ||
|
|
||
| if (this.shouldAnimate()) { | ||
| raf(() => { | ||
| this.state = AccordionState.Expanding; | ||
|
|
@@ -247,9 +311,14 @@ export class Accordion implements ComponentInterface { | |
| } | ||
| }; | ||
|
|
||
| private collapseAccordion = (initialUpdate = false) => { | ||
| private collapseAccordion = () => { | ||
| const { contentEl } = this; | ||
| if (initialUpdate || contentEl === undefined) { | ||
|
|
||
| /** | ||
| * If the content element isn't available yet, just set the state. | ||
| * This happens on initial render before the DOM is ready. | ||
| */ | ||
| if (contentEl === undefined) { | ||
| this.state = AccordionState.Collapsed; | ||
| return; | ||
| } | ||
|
|
@@ -291,6 +360,19 @@ export class Accordion implements ComponentInterface { | |
| * of what is set in the config. | ||
| */ | ||
| private shouldAnimate = () => { | ||
| /** | ||
| * Don't animate until after the first user interaction. | ||
| * This prevents animations on initial load when accordions | ||
| * start in an expanded or collapsed state programmatically. | ||
| * | ||
| * Additionally, don't animate the very first expansion even if | ||
| * hasInteracted is true. This handles edge cases like React StrictMode | ||
| * where effects run twice and might incorrectly mark as interacted. | ||
| */ | ||
| if (!this.hasInteracted || !this.hasEverBeenExpanded) { | ||
| return false; | ||
| } | ||
|
|
||
| if (typeof (window as any) === 'undefined') { | ||
| return false; | ||
| } | ||
|
|
@@ -312,7 +394,7 @@ export class Accordion implements ComponentInterface { | |
| return true; | ||
| }; | ||
|
|
||
| private updateState = async (initialUpdate = false) => { | ||
| private updateState = async () => { | ||
| const accordionGroup = this.accordionGroupEl; | ||
| const accordionValue = this.value; | ||
|
|
||
|
|
@@ -325,10 +407,10 @@ export class Accordion implements ComponentInterface { | |
| const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; | ||
|
|
||
| if (shouldExpand) { | ||
| this.expandAccordion(initialUpdate); | ||
| this.expandAccordion(); | ||
| this.isNext = this.isPrevious = false; | ||
| } else { | ||
| this.collapseAccordion(initialUpdate); | ||
| this.collapseAccordion(); | ||
|
|
||
| /** | ||
| * When using popout or inset, | ||
|
|
@@ -386,6 +468,12 @@ export class Accordion implements ComponentInterface { | |
|
|
||
| if (disabled || readonly) return; | ||
|
|
||
| /** | ||
| * Mark that the user has interacted with the accordion. | ||
| * This enables animations for all future state changes. | ||
| */ | ||
| this.hasInteracted = true; | ||
|
|
||
| if (accordionGroupEl) { | ||
| /** | ||
| * Because the accordion group may or may | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,6 +200,87 @@ it('should set default values if not provided', async () => { | |
| expect(accordion.classList.contains('accordion-collapsed')).toEqual(false); | ||
| }); | ||
|
|
||
| it('should not animate when initial value is set before load', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Item, Accordion, AccordionGroup], | ||
| }); | ||
|
|
||
| const accordionGroup = page.doc.createElement('ion-accordion-group'); | ||
| accordionGroup.innerHTML = ` | ||
| <ion-accordion value="first"> | ||
| <ion-item slot="header">Label</ion-item> | ||
| <div slot="content">Content</div> | ||
| </ion-accordion> | ||
| <ion-accordion value="second"> | ||
| <ion-item slot="header">Label</ion-item> | ||
| <div slot="content">Content</div> | ||
| </ion-accordion> | ||
| `; | ||
|
|
||
| accordionGroup.value = 'first'; | ||
| page.body.appendChild(accordionGroup); | ||
|
|
||
| await page.waitForChanges(); | ||
|
|
||
| const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; | ||
|
|
||
| expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); | ||
| expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't realize that spec tests capture these kinds of classes. I would have expected it to be false all the time since |
||
| }); | ||
|
|
||
| it('should not animate when initial value is set after load', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Item, Accordion, AccordionGroup], | ||
| }); | ||
|
|
||
| const accordionGroup = page.doc.createElement('ion-accordion-group'); | ||
| accordionGroup.innerHTML = ` | ||
| <ion-accordion value="first"> | ||
| <ion-item slot="header">Label</ion-item> | ||
| <div slot="content">Content</div> | ||
| </ion-accordion> | ||
| <ion-accordion value="second"> | ||
| <ion-item slot="header">Label</ion-item> | ||
| <div slot="content">Content</div> | ||
| </ion-accordion> | ||
| `; | ||
|
|
||
| page.body.appendChild(accordionGroup); | ||
| await page.waitForChanges(); | ||
|
|
||
| accordionGroup.value = 'first'; | ||
| await page.waitForChanges(); | ||
|
|
||
| const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; | ||
|
|
||
| expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); | ||
| expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); | ||
| }); | ||
|
|
||
| it('should not have animated class on first expansion', async () => { | ||
| const page = await newSpecPage({ | ||
| components: [Item, Accordion, AccordionGroup], | ||
| html: ` | ||
| <ion-accordion-group> | ||
| <ion-accordion value="first"> | ||
| <ion-item slot="header">Label</ion-item> | ||
| <div slot="content">Content</div> | ||
| </ion-accordion> | ||
| </ion-accordion-group> | ||
| `, | ||
| }); | ||
|
|
||
| const accordionGroup = page.body.querySelector('ion-accordion-group')!; | ||
| const firstAccordion = page.body.querySelector('ion-accordion[value="first"]')!; | ||
|
|
||
| // First expansion should not have the animated class | ||
| accordionGroup.value = 'first'; | ||
| await page.waitForChanges(); | ||
|
|
||
| expect(firstAccordion.classList.contains('accordion-animated')).toEqual(false); | ||
| expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); | ||
| }); | ||
|
|
||
| // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/27047 | ||
| it('should not have animated class when animated="false"', async () => { | ||
| const page = await newSpecPage({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| import { IonHeader, IonTitle, IonToolbar, IonPage, IonContent, IonAccordionGroup, IonAccordion, IonItem, IonLabel } from '@ionic/react'; | ||
| import { useEffect, useRef } from 'react'; | ||
|
|
||
| const AccordionGroup: React.FC = () => { | ||
| const accordionGroup = useRef<null | HTMLIonAccordionGroupElement>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (!accordionGroup.current) { | ||
| return; | ||
| } | ||
|
|
||
| accordionGroup.current.value = ['first', 'third']; | ||
| }, []); | ||
|
|
||
| return ( | ||
| <IonPage> | ||
| <IonHeader> | ||
| <IonToolbar> | ||
| <IonTitle>Accordion Group</IonTitle> | ||
| </IonToolbar> | ||
| </IonHeader> | ||
| <IonContent> | ||
| <IonAccordionGroup ref={accordionGroup} multiple={true}> | ||
| <IonAccordion value="first"> | ||
| <IonItem slot="header" color="light"> | ||
| <IonLabel>First Accordion</IonLabel> | ||
| </IonItem> | ||
| <div className="ion-padding" slot="content"> | ||
| First Content | ||
| </div> | ||
| </IonAccordion> | ||
| <IonAccordion value="second"> | ||
| <IonItem slot="header" color="light"> | ||
| <IonLabel>Second Accordion</IonLabel> | ||
| </IonItem> | ||
| <div className="ion-padding" slot="content"> | ||
| Second Content | ||
| </div> | ||
| </IonAccordion> | ||
| <IonAccordion value="third"> | ||
| <IonItem slot="header" color="light"> | ||
| <IonLabel>Third Accordion</IonLabel> | ||
| </IonItem> | ||
| <div className="ion-padding" slot="content"> | ||
| Third Content | ||
| </div> | ||
| </IonAccordion> | ||
| </IonAccordionGroup> | ||
| </IonContent> | ||
| </IonPage> | ||
| ); | ||
| }; | ||
|
|
||
| export default AccordionGroup; |
Uh oh!
There was an error while loading. Please reload this page.