Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export interface AccordionGroupChangeEventDetail<T = any> {
value: T;
initial?: boolean;
}

export interface AccordionGroupCustomEvent<T = any> extends CustomEvent {
Expand Down
74 changes: 45 additions & 29 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 @@ -73,33 +73,13 @@ export class AccordionGroup implements ComponentInterface {
*/
@Event() ionValueChange!: EventEmitter<AccordionGroupChangeEventDetail>;

private hasEmittedInitialValue = false;
private isUserInitiatedChange = false;

@Watch('value')
valueChanged() {
const { value, multiple } = this;

if (!multiple && Array.isArray(value)) {
/**
* We do some processing on the `value` array so
* that it looks more like an array when logged to
* the console.
* Example given ['a', 'b']
* Default toString() behavior: a,b
* Custom behavior: ['a', 'b']
*/
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 });
this.emitValueChange(false, this.isUserInitiatedChange);
this.isUserInitiatedChange = false;
}

@Watch('disabled')
Expand Down Expand Up @@ -184,11 +164,12 @@ export class AccordionGroup implements ComponentInterface {
* it is possible for the value to be set after the Web Component
* initializes but before the value watcher is set up in Stencil.
* As a result, the watcher callback may not be fired.
* We work around this by manually calling the watcher
* callback when the component has loaded and the watcher
* is configured.
* We work around this by manually emitting a value change when the component
* has loaded and the watcher is configured.
*/
this.valueChanged();
if (!this.hasEmittedInitialValue) {
this.emitValueChange(true);
}
}

/**
Expand All @@ -200,6 +181,7 @@ export class AccordionGroup implements ComponentInterface {
* accordion group to an array.
*/
private setValue(accordionValue: string | string[] | null | undefined) {
this.isUserInitiatedChange = true;
const value = (this.value = accordionValue);
this.ionChange.emit({ value });
}
Expand Down Expand Up @@ -276,6 +258,40 @@ export class AccordionGroup implements ComponentInterface {
return Array.from(this.el.querySelectorAll(':scope > ion-accordion')) as HTMLIonAccordionElement[];
}

private emitValueChange(initial: boolean, isUserInitiated = false) {
const { value, multiple } = this;

if (!multiple && Array.isArray(value)) {
/**
* We do some processing on the `value` array so
* that it looks more like an array when logged to
* the console.
* Example given ['a', 'b']
* Default toString() behavior: a,b
* Custom behavior: ['a', 'b']
*/
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
);
}

/**
* Track if this is the initial value update so accordions
* can skip transition animations when they first render.
*/
const shouldMarkInitial = initial || (!this.hasEmittedInitialValue && value !== undefined && !isUserInitiated);

this.ionValueChange.emit({ value, initial: shouldMarkInitial });

if (value !== undefined) {
this.hasEmittedInitialValue = true;
}
}

render() {
const { disabled, readonly, expand } = this;
const mode = getIonMode(this);
Expand Down
6 changes: 5 additions & 1 deletion core/src/components/accordion/accordion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { chevronDown } from 'ionicons/icons';

import { config } from '../../global/config';
import { getIonMode } from '../../global/ionic-global';
import type { AccordionGroupChangeEventDetail } from '../accordion-group/accordion-group-interface';

const enum AccordionState {
Collapsed = 1 << 0,
Expand Down Expand Up @@ -38,7 +39,10 @@ const enum AccordionState {
})
export class Accordion implements ComponentInterface {
private accordionGroupEl?: HTMLIonAccordionGroupElement | null;
private updateListener = () => this.updateState(false);
private updateListener = (ev: CustomEvent<AccordionGroupChangeEventDetail>) => {
const initialUpdate = ev.detail?.initial ?? false;
this.updateState(initialUpdate);
};
private contentEl: HTMLDivElement | undefined;
private contentElWrapper: HTMLDivElement | undefined;
private headerEl: HTMLDivElement | undefined;
Expand Down
100 changes: 100 additions & 0 deletions core/src/components/accordion/test/accordion.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { newSpecPage } from '@stencil/core/testing';

import type { AccordionGroupChangeEventDetail } from '../../accordion-group/accordion-group-interface';
import { AccordionGroup } from '../../accordion-group/accordion-group';
import { Item } from '../../item/item';
import { Accordion } from '../accordion';
Expand Down Expand Up @@ -200,6 +201,105 @@ 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>
`;

const details: AccordionGroupChangeEventDetail[] = [];
accordionGroup.addEventListener('ionValueChange', (event: CustomEvent<AccordionGroupChangeEventDetail>) => {
details.push(event.detail);
});

accordionGroup.value = 'first';
page.body.appendChild(accordionGroup);

await page.waitForChanges();

expect(details[0]?.initial).toBe(true);

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>
`;

const details: AccordionGroupChangeEventDetail[] = [];
accordionGroup.addEventListener('ionValueChange', (event: CustomEvent<AccordionGroupChangeEventDetail>) => {
details.push(event.detail);
});

page.body.appendChild(accordionGroup);
await page.waitForChanges();

accordionGroup.value = 'first';
await page.waitForChanges();

const firstDetail = details.find((detail) => detail.value === 'first');
expect(firstDetail?.initial).toBe(true);

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 animate when accordion is first opened by user', 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 details: AccordionGroupChangeEventDetail[] = [];
accordionGroup.addEventListener('ionValueChange', (event: CustomEvent<AccordionGroupChangeEventDetail>) => {
details.push(event.detail);
});

await accordionGroup.requestAccordionToggle('first', true);
await page.waitForChanges();

const lastDetail = details[details.length - 1];
expect(lastDetail?.initial).toBe(false);
});

// 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
Loading