Skip to content

refactor(accordion, accordion-item): getElementProp on child is factored out and parent gets a mutation observer to update children #6055

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.

Sign up for GitHub

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

Closed
Elijbet wants to merge 14 commits into from
Closed

refactor(accordion, accordion-item): getElementProp on child is factored out and parent gets a mutation observer to update children #6055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3b18af1
fix(accordion, accordion-item): getElementProp on child is factored o…
Elijbet Dec 15, 2022
2a59f04
Merge branch 'master' into elijbet/6038-accordion-item-refactor-getEl…
Elijbet Dec 15, 2022
c0a37e7
merge conflicts and refactor
Elijbet Dec 16, 2022
d01eb60
merge conflicts resolved
Elijbet Dec 16, 2022
c598df4
merge files added
Elijbet Dec 16, 2022
7f49d70
cleanup
Elijbet Dec 16, 2022
75cda06
merge conflicts resolved
Elijbet Dec 18, 2022
43eae42
type correction for private passPropsToAccordionItems
Elijbet Dec 18, 2022
47001c5
e2e tests for icon behavior
Elijbet Dec 18, 2022
c2d904e
merged files
Elijbet Dec 18, 2022
ddf6c34
cleanup
Elijbet Dec 18, 2022
4fbf745
merge master
Elijbet Dec 19, 2022
2d5b3e2
WIP: cleanups
Elijbet Dec 19, 2022
abf837e
merge master
Elijbet Dec 19, 2022
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
35 changes: 25 additions & 10 deletions src/components/accordion-item/accordion-item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import {
Host,
Listen,
Prop,
VNode
VNode,
Watch
} from "@stencil/core";
import { getElementDir, getElementProp, getSlotted, toAriaBoolean } from "../../utils/dom";
import {
Expand All @@ -17,7 +18,7 @@ import {
} from "../../utils/conditionalSlot";
import { CSS_UTILITY } from "../../utils/resources";
import { SLOTS, CSS } from "./resources";
import { FlipContext, Position } from "../interfaces";
import { FlipContext, Position, Scale } from "../interfaces";
import { RegistryEntry, RequestedItem } from "./interfaces";

/**
Expand All @@ -43,23 +44,35 @@ export class AccordionItem implements ConditionalSlotComponent {
//
//--------------------------------------------------------------------------

/** Specifies a description for the component. */
@Prop() description: string;

/** When `true`, the component is expanded. */
@Prop({ reflect: true, mutable: true }) expanded = false;

/** Specifies heading text for the component. */
@Prop() heading: string;

/** Specifies a description for the component. */
@Prop() description: string;
/** When `true`, the icon will be flipped when the element direction is right-to-left (`"rtl"`). */
@Prop({ reflect: true }) iconFlipRtl: FlipContext;

/** Specifies an icon to display at the start of the component. */
@Prop({ reflect: true }) iconStart: string;

/** Specifies an icon to display at the end of the component. */
@Prop({ reflect: true }) iconEnd: string;

/** When `true`, the icon will be flipped when the element direction is right-to-left (`"rtl"`). */
@Prop({ reflect: true }) iconFlipRtl: FlipContext;
/**
* Specifies the size of the component inherited from the `accordion`.
*
* @internal
*/
@Prop({ reflect: true }) scale: Scale = "m";
Comment thread
Elijbet marked this conversation as resolved.

@Watch("scale")
onScaleChange(): void {
this.internalIconScale = this.scale === "l" ? "m" : "s";
}

//--------------------------------------------------------------------------
//
Expand Down Expand Up @@ -142,16 +155,15 @@ export class AccordionItem implements ConditionalSlotComponent {
flipRtl={iconFlipRtl === "both" || iconFlipRtl === "start"}
icon={this.iconStart}
key="icon-start"
scale="s"
scale={this.internalIconScale}
/>
) : null;
const iconEndEl = this.iconEnd ? (
<calcite-icon
class={CSS.iconEnd}
flipRtl={iconFlipRtl === "both" || iconFlipRtl === "end"}
icon={this.iconEnd}
key="icon-end"
scale="s"
scale={this.internalIconScale}
/>
) : null;
const { description } = this;
Expand Down Expand Up @@ -191,7 +203,7 @@ export class AccordionItem implements ConditionalSlotComponent {
? "minus"
: "plus"
}
scale="s"
scale={this.internalIconScale}
/>
</div>
{this.renderActionsEnd()}
Expand Down Expand Up @@ -258,6 +270,9 @@ export class AccordionItem implements ConditionalSlotComponent {
/** what icon type does the parent accordion specify */
private iconType: string;

/** size of the component to be inherited from the `accordion` */
private internalIconScale: Scale;

/** handle clicks on item header */
private itemHeaderClickHandler = (): void => this.emitRequestedItem();
//--------------------------------------------------------------------------
Expand Down
16 changes: 8 additions & 8 deletions src/components/accordion-item/resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ export const SLOTS = {
};

export const CSS = {
icon: "icon",
header: "header",
headerContent: "header-content",
actionsStart: "actions-start",
actionsEnd: "actions-end",
headerText: "header-text",
heading: "heading",
content: "content",
description: "description",
expandIcon: "expand-icon",
content: "content",
header: "header",
headerContainer: "header-container",
headerContent: "header-content",
headerText: "header-text",
heading: "heading",
icon: "icon",
iconStart: "icon--start",
iconEnd: "icon--end",
headerContainer: "header-container"
iconEnd: "icon--end"
};
71 changes: 53 additions & 18 deletions src/components/accordion/accordion.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { newE2EPage } from "@stencil/core/testing";
import { E2EElement, E2EPage, newE2EPage } from "@stencil/core/testing";
import { accessible, renders, hidden } from "../../tests/commonTests";
import { html } from "../../../support/formatting";
import { CSS } from "../accordion-item/resources";
Expand Down Expand Up @@ -47,23 +47,58 @@ describe("calcite-accordion", () => {
expect(element).toEqualAttribute("icon-type", "caret");
});

it("renders icon if requested", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-accordion appearance="minimal" icon-position="start" scale="l" selection-mode="single-persist" icon-type="caret">
<calcite-accordion-item heading="Accordion Title 1" icon-start="car" id="1">Accordion Item Content
</calcite-accordion-item>
<calcite-accordion-item heading="Accordion Title 1" id="2" expanded>Accordion Item Content
</calcite-accordion-item>
<calcite-accordion-item heading="Accordion Title 3" icon-start="car" id="3">Accordion Item Content
</calcite-accordion-item>
</calcite-accordion>`);
const icon1 = await page.find(`calcite-accordion-item[id='1'] >>> .${CSS.iconStart}`);
const icon2 = await page.find(`calcite-accordion-item[id='2'] >>> .${CSS.iconStart}`);
const icon3 = await page.find(`calcite-accordion-item[id='3'] >>> .${CSS.iconStart}`);
expect(icon1).not.toBe(null);
expect(icon2).toBe(null);
expect(icon3).not.toBe(null);
describe("icon behavior", () => {
let page: E2EPage;
Comment thread
Elijbet marked this conversation as resolved.
const scale = { l: "l", m: "m", s: "s" };
const accordionItemContent = html`<calcite-accordion-item icon-start="car" id="1"></calcite-accordion-item>
<calcite-accordion-item id="2"></calcite-accordion-item>
<calcite-accordion-item icon-start="car" id="3"></calcite-accordion-item>`;

beforeEach(async () => {
page = await newE2EPage();
await page.setContent(html`<calcite-accordion scale="l"> ${accordionItemContent} </calcite-accordion>`);
await page.waitForChanges();
});

it("renders icon if requested", async () => {
const icon1: E2EElement = await page.find(`calcite-accordion-item[id='1'] >>> .${CSS.iconStart}`);
const icon2: E2EElement = await page.find(`calcite-accordion-item[id='2'] >>> .${CSS.iconStart}`);
const icon3: E2EElement = await page.find(`calcite-accordion-item[id='3'] >>> .${CSS.iconStart}`);
expect(icon1).not.toBe(null);
expect(icon2).toBe(null);
expect(icon3).not.toBe(null);
});

it("renders m scale icon for l scale accordion-item", async () => {
const item1: E2EElement = await page.find(`calcite-accordion-item[id='1']`);
expect(await item1.getProperty("scale")).toBe(scale.l);
const icon1: E2EElement = await page.find(`calcite-accordion-item[id='1'] >>> .${CSS.iconStart}`);
expect(await icon1.getProperty("scale")).toBe(scale.m);
});

it("renders corresponding scale on accordion-item when parent scale changes, icon scale not affected", async () => {
const accordion: E2EElement = await page.find("calcite-accordion");
await accordion.setProperty("scale", scale.s);
await page.waitForChanges();

const item1: E2EElement = await page.find(`calcite-accordion-item[id='1']`);
expect(await item1.getProperty("scale")).toEqual(scale.s);

const icon1: E2EElement = await page.find(`calcite-accordion-item[id='1'] >>> .${CSS.iconStart}`);
expect(await icon1.getProperty("scale")).toEqual(scale.m);
Copy link
Copy Markdown
Contributor

@macandcheese macandcheese Dec 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should we instead expect this icon be a scale s based on the scale s of the parent calcite-accordion-item?

Copy link
Copy Markdown
Contributor Author

@Elijbet Elijbet Dec 19, 2022

Choose a reason for hiding this comment

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

We should probably add this to tomorrow's Pattern Discussion: Parent <> Child Property Relationships. From what I understand while we expect a parent to update children where these 2 have a parent/child relationship (as in accordion and its item), element and its slotted icon don't have that relationship and are expected to have independent scales. But in this particular case, it does feel a bit odd. Erik also brought up that we should not override an item with the parent if a user sets it individually to a different scale, although I'm not sure what the use case for that would be.

Copy link
Copy Markdown
Contributor

@eriklharper eriklharper Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Erik also brought up that we should not override an item with the parent if a user sets it individually to a different scale, although I'm not sure what the use case for that would be.

Not sure what the specific use case would be either, but I still think that if the scale property is explicitly applied either as an attribute or by setting the property directly on the child component, that this value should be honored and not overridden by the parent component. We can actively discourage doing this in the documentation, and encourage users to set the value at the parent level to ensure visual consistency, but I don't think we should break from convention with regards to how property values are honored at the individual component level.

});

it("renders expected scale icon on child when scale is set on child level (no parent override)", async () => {
const accordion: E2EElement = await page.find("calcite-accordion");
expect(await icon1.getProperty("scale")).toEqual(scale.l);

const item1: E2EElement = await page.find(`calcite-accordion-item[id='1']`);
await item1.setProperty("scale", scale.m);
await page.waitForChanges();

expect(await accordion.getProperty("scale")).toEqual(scale.l);
expect(await item1.getProperty("scale")).toEqual(scale.m);
Copy link
Copy Markdown
Contributor

@macandcheese macandcheese Dec 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we want the scale of calcite-accordion and calcite-accordion-item to match in this case - shouldn't let folks set scale "m" on the child if the parent is "l". (assuming scale on calcite-accordion-item is internal property)

Copy link
Copy Markdown
Contributor Author

@Elijbet Elijbet Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Seems logical that if calcite-accordion-item is an internal property we shouldn't be letting users set it to a different scale, but based on other comments looks like this is also part of tomorrow's Child Property Relationships discussion.

Copy link
Copy Markdown
Contributor

@eriklharper eriklharper Dec 19, 2022

Choose a reason for hiding this comment

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

Don't we want the scale of calcite-accordion and calcite-accordion-item to match in this case - shouldn't let folks set scale "m" on the child if the parent is "l". (assuming scale on calcite-accordion-item is internal property)

I agree it seems odd to allow this, but if you think about it, this pattern breaks component autonomy. When a component defines its own scale prop, I as a developer expect it to honor whatever value I'm explicitly setting it to, excepting the default fallback value. If a parent value comes in and overrides that value I've explicitly set on the child, the property pattern on that child component no longer behaves according to standard convention.

Copy link
Copy Markdown
Contributor

@macandcheese macandcheese Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, although if the property is internal and is set by / managed by the parent maybe I wouldn't expect to be able to set this at all, as its undocumented. Though to follow through on that the scale probably shouldn't have the default to "m" until it receives the scale from calcite-accordion.

Copy link
Copy Markdown
Contributor

@eriklharper eriklharper Dec 19, 2022

Choose a reason for hiding this comment

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

This same principle applies to object inheritance in programming. If I have Object A that defines a foo property, an Object B that extends Object A, I don't expect setting the foo property on Object A to override what I've explicitly set it to on Object B. I expect the lowest level of specificity on that property to win above all others.

Copy link
Copy Markdown
Contributor

@macandcheese macandcheese Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Right - but its not a public property (or, is proposed to be an undocumented, internal property) - so the test case of it being set in that way should not be valid?

Copy link
Copy Markdown
Contributor

@eriklharper eriklharper Dec 19, 2022

Choose a reason for hiding this comment

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

Yeah, although if the property is internal and is set by / managed by the parent maybe I wouldn't expect to be able to set this at all, as its undocumented. Though to follow through on that the scale probably shouldn't have the default to "m" until it receives the scale from calcite-accordion.

For sure. I think with a component like accordion, its children items are so tightly interwoven with the parent that this pattern does make sense where you would explicitly control children properties like scale from the parent level. It gets trickier when you have child components that can live standalone by themselves like radio-button and tile-select where they need to work on their own regardless if their corresponding parent wrapper component is being used.

Right - but its not a public property (or, is proposed to not be an undocumented, internal property) - so the test case of it being set in that way should not be valid?

Agreed. I think if we mark these properties as internal, and call out in our documentation that modifying internal properties is not supported, then this pattern can be adopted for certain parent/child component systems.

});
});

it("renders expanded item based on attribute in dom", async () => {
Expand Down
41 changes: 38 additions & 3 deletions src/components/accordion/accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
import { Component, Element, Event, EventEmitter, h, Listen, Prop, VNode } from "@stencil/core";
import {
Component,
Element,
Event,
EventEmitter,
h,
Listen,
Prop,
VNode,
Watch
} from "@stencil/core";
import { RequestedItem } from "./interfaces";
import { Appearance, Position, Scale } from "../interfaces";
import { createObserver } from "../../utils/observers";
import { SelectionMode } from "../interfaces";
/**
* @slot - A slot for adding `calcite-accordion-item`s. `calcite-accordion` cannot be nested, however `calcite-accordion-item`s can.
Expand Down Expand Up @@ -40,6 +51,11 @@ export class Accordion {
/** Specifies the size of the component. */
@Prop({ reflect: true }) scale: Scale = "m";

@Watch("scale")
onScaleChange(): void {
this.passPropsToAccordionItems();
}

/**
* Specifies the selection mode - `"multiple"` (allow any number of open items), `"single"` (allow one open item),
* or `"single-persist"` (allow and require one open item).
Expand All @@ -66,13 +82,22 @@ export class Accordion {
//
//--------------------------------------------------------------------------

connectedCallback(): void {
this.passPropsToAccordionItems();
this.mutationObserver?.observe(this.el, { childList: true, subtree: true });
}

componentDidLoad(): void {
if (!this.sorted) {
this.items = this.sortItems(this.items);
this.sorted = true;
}
}

disconnectedCallback(): void {
this.mutationObserver?.disconnect();
}

render(): VNode {
const transparent = this.appearance === "transparent";
const minimal = this.appearance === "minimal";
Expand Down Expand Up @@ -126,11 +151,13 @@ export class Accordion {
/** created list of Accordion items */
private items = [];

/** keep track of the requested item for multi mode */
private requestedAccordionItem: HTMLCalciteAccordionItemElement;

/** keep track of whether the items have been sorted so we don't re-sort */
private sorted = false;

/** keep track of the requested item for multi mode */
private requestedAccordionItem: HTMLCalciteAccordionItemElement;
mutationObserver = createObserver("mutation", () => this.passPropsToAccordionItems());

//--------------------------------------------------------------------------
//
Expand All @@ -140,4 +167,12 @@ export class Accordion {

private sortItems = (items: any[]): any[] =>
items.sort((a, b) => a.position - b.position).map((a) => a.item);

private passPropsToAccordionItems = (): void => {
(
Array.from(
Copy link
Copy Markdown
Contributor

@macandcheese macandcheese Dec 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we use the existing this.items instead of querying for the child items again? I suppose this.items may not always be populated at connectedCallback but should be in componentDidLoad

componentDidLoad(): void {
     ...
    this.passPropsToAccordionItems();
  }

and

private passPropsToAccordionItems = (): void => {
    this.items.forEach((el) => (el.scale = this.scale));
};

Seems to work reliably.

Copy link
Copy Markdown
Contributor

@eriklharper eriklharper Dec 19, 2022

Choose a reason for hiding this comment

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

This makes sense as long as this.items gets updated when accordion-items get added or removed from the light dom. Is this currently handled in any way?

Copy link
Copy Markdown
Contributor

@macandcheese macandcheese Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah good point the internal registration events are kind of an outdated pattern and wouldn't respond - slot change observer that responds to addition / removal of children and updates this.items is probably better. There's a handful of our ... more senior 👴👵 ... components that could probably benefit from a change like that.

this.el.querySelectorAll("calcite-accordion-item") as any
) as HTMLCalciteAccordionItemElement[]
).forEach((accordionItem) => (accordionItem.scale = this.scale));
};
}