refactor(accordion, accordion-item): `getElementProp` on child is factored out and parent gets a mutation observer to update children by Elijbet · Pull Request #6055 · Esri/calcite-design-system

Elijbet · 2022-12-15T04:47:43Z

Related Issue: #6038, #5698

Summary

getElementProp on child is factored out and parent gets a mutation observer to update children.

…ut and parent gets a mutation observer to update children

…ementProp-and-get-mutation-observer-on-parent

Elijbet · 2022-12-15T04:50:48Z

I implemented this new pattern to just the scale property, this is necessary to move ahead with #5698.
I can merge this as is with just scale and continue working on accordion-item for #5698, and make a follow-up issue to address all other properties that need to be inherited.

Or I can put all other properties that need to be inherited in this PR as well since most of the work is done. I would just need to know which additional ones need to be inherited (appearance, iconPosition, iconType).

Added "[Erik/Eliza]: Pattern Discussion: Parent <> Child Property Relationships" for the next meeting.

@driskull @eriklharper

macandcheese · 2022-12-18T21:41:22Z

+
+  private passPropsToAccordionItems = (): void => {
+    (
+      Array.from(


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.

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?

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.

macandcheese · 2022-12-18T21:55:26Z

+      await page.waitForChanges();
+
+      expect(await accordion.getProperty("scale")).toEqual(scale.l);
+      expect(await item1.getProperty("scale")).toEqual(scale.m);


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)

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.

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.

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.

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.

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?

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.

macandcheese · 2022-12-18T21:56:01Z

+      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);


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

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.

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.

github-actions · 2022-12-27T01:55:10Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

Elijbet · 2023-07-08T02:20:22Z

Closing this, as it was resolved in #7191.

Elijbet added 2 commits December 14, 2022 20:44

fix(accordion, accordion-item): getElementProp on child is factored o…

3b18af1

…ut and parent gets a mutation observer to update children

Merge branch 'master' into elijbet/6038-accordion-item-refactor-getEl…

2a59f04

…ementProp-and-get-mutation-observer-on-parent

github-actions Bot assigned Elijbet Dec 15, 2022

github-actions Bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Dec 15, 2022

Elijbet changed the title ~~fix(accordion, accordion-item): getElementProp on child is factored out and parent gets a mutation observer to update children~~ fix(accordion, accordion-item): getElementProp on child is factored out and parent gets a mutation observer to update children Dec 15, 2022

driskull reviewed Dec 15, 2022

View reviewed changes

Comment thread src/components/accordion/accordion.tsx Outdated

Elijbet added 9 commits December 16, 2022 11:16

merge conflicts and refactor

c0a37e7

merge conflicts resolved

d01eb60

merge files added

c598df4

cleanup

7f49d70

merge conflicts resolved

75cda06

type correction for private passPropsToAccordionItems

43eae42

e2e tests for icon behavior

47001c5

merged files

c2d904e

cleanup

ddf6c34

macandcheese reviewed Dec 18, 2022

View reviewed changes

Comment thread src/components/accordion-item/accordion-item.tsx

macandcheese reviewed Dec 18, 2022

View reviewed changes

Comment thread src/components/accordion/accordion.e2e.ts Outdated

macandcheese reviewed Dec 18, 2022

View reviewed changes

Comment thread src/components/accordion/accordion.e2e.ts

macandcheese reviewed Dec 18, 2022

View reviewed changes

Comment thread src/components/accordion-item/accordion-item.tsx Outdated

Elijbet added 3 commits December 19, 2022 09:45

merge master

4fbf745

WIP: cleanups

2d5b3e2

merge master

abf837e

github-actions Bot added the Stale Issues or pull requests that have not had recent activity. label Dec 27, 2022

Elijbet closed this Jul 8, 2023

Elijbet deleted the elijbet/6038-accordion-item-refactor-getElementProp-and-get-mutation-observer-on-parent branch February 29, 2024 19:06

Conversation

Elijbet commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Elijbet commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

macandcheese Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriklharper Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

macandcheese Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

macandcheese Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Elijbet Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriklharper Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

macandcheese Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriklharper Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

macandcheese Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriklharper Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

macandcheese Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Elijbet Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

eriklharper Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Dec 27, 2022

Uh oh!

Elijbet commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Elijbet commented Dec 15, 2022 •

edited

Loading

Elijbet commented Dec 15, 2022 •

edited

Loading

macandcheese Dec 18, 2022 •

edited

Loading

macandcheese Dec 19, 2022 •

edited

Loading

macandcheese Dec 18, 2022 •

edited

Loading

Elijbet Dec 19, 2022 •

edited

Loading

macandcheese Dec 19, 2022 •

edited

Loading

macandcheese Dec 19, 2022 •

edited

Loading

macandcheese Dec 18, 2022 •

edited

Loading

eriklharper Dec 19, 2022 •

edited

Loading