- 
                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
base: main
Are you sure you want to change the base?
Changes from all commits
58c2a8a
              761d06f
              1a1dcda
              2095f2f
              63f32a5
              cc125e6
              90ebfba
              e345efd
              f84c484
              0ce05dc
              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 = () => { | ||||||||||||||||||||||
| 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. | ||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * 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; | ||||||||||||||||||||||
| const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; | ||||||||||||||||||||||
| const isExpanded = this.state === AccordionState.Expanded || this.state === AccordionState.Expanding; | ||||||||||||||||||||||
| const stateWillChange = shouldExpand !== isExpanded; | ||||||||||||||||||||||
| 
      Comment on lines
    
      +48
     to 
      +52
    
   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. Nit: It might make more sense to clarify which value belongs to the group, since I'd expect  
        Suggested change
       
 | ||||||||||||||||||||||
|  | ||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * 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) { | ||||||||||||||||||||||
| 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. 
        Suggested change
       
 See above | ||||||||||||||||||||||
| 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); | ||
| 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; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revert the changes to this file since it doesn't do anything?