Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions core/src/components/accordion-group/accordion-group.tsx
Copy link
Member

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?

Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,12 @@ export class AccordionGroup implements ComponentInterface {
*/
printIonWarning(
`[ion-accordion-group] - An array of values was passed, but multiple is "false". This is incorrect usage and may result in unexpected behaviors. To dismiss this warning, pass a string to the "value" property when multiple="false".

Value Passed: [${value.map((v) => `'${v}'`).join(', ')}]
`,
this.el
);
}

/**
* Do not use `value` here as that will be
* not account for the adjustment we make above.
*/
this.ionValueChange.emit({ value: this.value });
}

Expand Down
106 changes: 97 additions & 9 deletions core/src/components/accordion/accordion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,40 @@ const enum AccordionState {
})
export class Accordion implements ComponentInterface {
private accordionGroupEl?: HTMLIonAccordionGroupElement | null;
private updateListener = () => this.updateState(false);
private updateListener = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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 value to refer to the current component:

Suggested change
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;


/**
* 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (value !== undefined) {
if (groupValue !== undefined) {

See above

this.hasReceivedFirstUpdate = true;
}
}

this.updateState();
};
private contentEl: HTMLDivElement | undefined;
private contentElWrapper: HTMLDivElement | undefined;
private headerEl: HTMLDivElement | undefined;
Expand All @@ -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
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;

Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down
81 changes: 81 additions & 0 deletions core/src/components/accordion/test/accordion.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 expect would run after the animation ended. I'll have to keep this in mind for future tests. Thanks!

});

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({
Expand Down
2 changes: 2 additions & 0 deletions packages/react/test/base/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import KeepContentsMounted from './pages/overlay-components/KeepContentsMounted'
import OverlayComponents from './pages/overlay-components/OverlayComponents';
import OverlayHooks from './pages/overlay-hooks/OverlayHooks';
import ReorderGroup from './pages/ReorderGroup';
import AccordionGroup from './pages/AccordionGroup';

setupIonicReact();

Expand Down Expand Up @@ -69,6 +70,7 @@ const App: React.FC = () => (
<Route path="/icons" component={Icons} />
<Route path="/inputs" component={Inputs} />
<Route path="/reorder-group" component={ReorderGroup} />
<Route path="/accordion-group" component={AccordionGroup} />
</IonRouterOutlet>
</IonReactRouter>
</IonApp>
Expand Down
54 changes: 54 additions & 0 deletions packages/react/test/base/src/pages/AccordionGroup.tsx
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;
3 changes: 3 additions & 0 deletions packages/react/test/base/src/pages/Main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ const Main: React.FC<MainProps> = () => {
</IonHeader>
<IonContent>
<IonList>
<IonItem routerLink="/accordion-group">
<IonLabel>Accordion Group</IonLabel>
</IonItem>
<IonItem routerLink="/overlay-hooks">
<IonLabel>Overlay Hooks</IonLabel>
</IonItem>
Expand Down
Loading