fix(accordion, accordion-item): `icon-position`, `icon-type`, `selection-mode` and `scale` can now be set as props or attributes by Elijbet · Pull Request #7191 · Esri/calcite-design-system

Elijbet · 2023-06-17T00:17:05Z

Related Issue: #6199, #6038, #6200

Summary

This should resolve #6199, where you're able to set accordionEl.setAttribute("icon-position", "start"), but not accordionEl.iconPosition = "start", happening because of a logical issue within getElementProp function.

getElementProp is being refactored across child components as an outdated pattern in #6038, so this will also kick off that issue as well.

Instead of the accordion-item looking up the parent for iconPosition, iconType, selectionMode, or scale, these get set by the accordion parent.

The logic for setting these props thus moves to the parent, getting rid of the getElementProp altogether. The parent component gets a mutation observer to do this as well as watchers for when it needs to modify the children.

With the mutationObserver added, there is no longer a need for the calciteInternalAccordionItemRegister event logic.

…dion-item

driskull

Looking good! Not sure what the issue is. Can you debug and see where the failure is occuring?

driskull · 2023-06-17T00:32:01Z

@@ -98,10 +128,6 @@ export class AccordionItem implements ConditionalSlotComponent {

  connectedCallback(): void {
    this.parent = this.el.parentElement as HTMLCalciteAccordionElement;


The parent could probably be set as well.

Hmm... not sure how to achieve this. What do you have in mind?

What I was thinking is that the owning accordion could set itself on the items as the parent. Just like how its setting the icontype, iconPosition, scale.

…te and correct location of the attributes

driskull

Looking good @Elijbet ✨ 👍

driskull · 2023-06-20T16:25:03Z

@@ -98,10 +128,6 @@ export class AccordionItem implements ConditionalSlotComponent {

  connectedCallback(): void {
    this.parent = this.el.parentElement as HTMLCalciteAccordionElement;


What I was thinking is that the owning accordion could set itself on the items as the parent. Just like how its setting the icontype, iconPosition, scale.

… on children, cleanup

…dion-item

…ed and it's position, which is now passed in as an argument instead of being an internal prop

jcfranco · 2023-06-26T15:50:42Z

This should resolve #6199

This means this PR is a fix and no longer a refactor. 🥳

…dion-item

…t coverage for #6199

…e the use of position

Suggestions have been implemented!

benelan

I have a couple quick questions questions. Also can you change the PR title to be about #6199 rather than the refactor, since the fix is what will show up in the changelog

benelan · 2023-06-26T19:22:43Z

+      const accordionItemsQueryArray = Array.from(
+        this.el.querySelectorAll("calcite-accordion-item")
+      );
+      this.accordionItems = accordionItemsQueryArray;


What is the purpose of pushing addedAccordionItems into accordionItems if accordionItems is being re-referenced to accordionItemsQueryArray directly afterwards?

Looks like it isn't needed anymore. Removing position removed the need for accordion items being an array of objects, which makes that bit redundant. When we queried items they had to be mapped to the accordionItem[].

benelan · 2023-06-26T20:52:20Z

-        ) as HTMLCalciteAccordionItemElement;
-      }
-    }
-    this.updateAccordionItems(addedAccordionItem);


Hmm idk how I feel about this. Removing this part will have a performance impact because now the DOM is being queried on every mutation, whereas before it was only being queried when nodes were added.

querySelectorAll is a pretty intensive task, especially for something like accordion which can have a yuuge amount of child nodes.

Do we really need to query the DOM at all? We should be able to get the added/removed/target nodes from the mutation observer right

updateAccordionItems() runs also on watcher for a number of props that are set by parents on children. We need to keep track of items for updateAccordionItems to map set props/prop changes onto them. Looks like at this point that's all updateAccordionItems is doing, as we no longer keep track of position, or items being added because of the query.

Mutation observer track additions, removals, and attribute changes, right? If that's the case why do we even need a watcher? We could just run the update function to sync props on component load in addition to the observer, but we'd need to query still.

Yeah now that the constraint of only querying when the mutation included addedNodes is removed, I'm pretty sure the watchers are redundant.

However I'm still concerned about using querySelectorAll on every mutation. There has to be a better way to do this that doesn't have the potential to explode someone's computer. Found this explaination after some googling: https://stackoverflow.com/a/39332340

Nevermind, it looks like the MutationObserver isn't set up to observe attributes so I think we would still need to watchers

Also it's not watching the whole subtree so the performance hit won't be too bad, I guess. Does the current logic cover nested accordions or accordion items?

@benelan I think this should be fine in terms of perf. The MutationObserver is configured to only run on direct child changes (see MutationObserver/observe#childList).

jcfranco

Once the review comments have been addressed, this should be good to merge! 🚀

…on-item

…ionItems

…on-item

* origin/main: chore: release next fix(block): loader now appears for all loading cases (#7303) build: update browserslist db (#7301) build(deps): Bump eslint-plugin-jsdoc from 46.2.6 to 46.4.3 (#7306) build(deps): Bump @floating-ui/dom from 1.4.3 to 1.4.4 in /packages/calcite-components (#7305) build(deps): Bump chromatic from 6.19.5 to 6.19.9 (#7308) build(deps): Bump cpy-cli from 4.2.0 to 5.0.0 (#7281) chore: release next feat(text-area): provide additional context for AT users when character limit exceeds (#7299) build(deps): Bump stylelint from 14.16.1 to 15.10.1 (#7296) chore: release next ci: allow lint-staged to find typescript parser config file (#7297) fix(accordion, accordion-item): `icon-position`, `icon-type`, `selection-mode` and `scale` can now be set as props or attributes (#7191) test(stack): stabilize screenshot tests (#7298) build: make sure eslint-plugin-calcite-components is set up correctly (#7294) chore: add translations (#7290) refactor: Ensure all setFocus methods return calls to setFocus (#7287) chore: release next fix(card): ensure teardown logic is called when disconnected (#7289) test: stabilize tests using setFocus (#7295)

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-components: 1.5.0</summary> ## [1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@1.4.3...@esri/calcite-components@1.5.0) (2023-08-03) ### Features * **action-group:** Adds overlayPositioning property. ([#7366](#7366)) ([ca9f35a](ca9f35a)) * Allow sharing focus trap stacks via configuration global ([#7334](#7334)) ([934a19f](934a19f)) * Automatically import and define Calcite Components when importing their React wrapper ([#7185](#7185)) ([bf0ff67](bf0ff67)) * **block, block-section:** Add setFocus method ([#7208](#7208)) ([35d4bbb](35d4bbb)) * **block:** Improve block's content layout to allow scrolling ([#7367](#7367)) ([ecbf17b](ecbf17b)) * **color-picker:** Replaces thumb focus outline to rounded ([#7378](#7378)) ([d803980](d803980)) * **filter:** Add filter method ([#7127](#7127)) ([5a4283f](5a4283f)) * **flow:** Adds setFocus method ([#7252](#7252)) ([2472c58](2472c58)) * Improve focus behavior in components ([#7277](#7277)) ([ad9fbca](ad9fbca)) * **input-time-zone:** Add input-time-zone component ([#6947](#6947)) ([87bd496](87bd496)) * **list:** Add slots for filter actions ([#7183](#7183)) ([da07ab1](da07ab1)) * **list:** Add support for dragging items. ([#7109](#7109)) ([7324f70](7324f70)) * **menu-item:** Update spacing and icon layout ([#7381](#7381)) ([5659671](5659671)) * **navigation-logo:** Increase font-size of heading with no description ([#7081](#7081)) ([355e101](355e101)) * **switch:** Updates focus outline to be rounded ([#7390](#7390)) ([2616b82](2616b82)) * **text-area:** Provide additional context for AT users when character limit exceeds ([#7299](#7299)) ([c5678eb](c5678eb)) * **text-area:** Provide additional context for AT users when character limit exceeds ([#7412](#7412)) ([c1af3c7](c1af3c7)) ### Bug Fixes * **accordion, accordion-item:** `icon-position`, `icon-type`, `selection-mode` and `scale` can now be set as props or attributes ([#7191](#7191)) ([2b09aba](2b09aba)) * **action-bar:** No longer delegates focus when clicked on non-focusable region ([#7310](#7310)) ([1a9c15c](1a9c15c)) * **action:** Correctly focus the button after rendering updates. ([#7255](#7255)) ([40fe2ce](40fe2ce)) * **block:** Loader now appears for all loading cases ([#7303](#7303)) ([5af3600](5af3600)) * **block:** Removes extra loading indicator ([#7239](#7239)) ([a334a75](a334a75)) * **card:** Ensure teardown logic is called when disconnected ([#7289](#7289)) ([d07e322](d07e322)) * **chip:** Disconnect mutation observer when component is disconnected from the DOM ([#7418](#7418)) ([412e5fb](412e5fb)) * **color-picker:** Draw slider thumbs within bounds ([#7398](#7398)) ([2f37854](2f37854)) * **color-picker:** Fix opacity slider keyboard nudging ([#7400](#7400)) ([2b4f7c3](2b4f7c3)) * **color-picker:** Maintains correct numbering system when entering invalid RGB value ([#7327](#7327)) ([8d2a3a5](8d2a3a5)) * **combobox:** Add space after grouped items ([#7302](#7302)) ([b1580c7](b1580c7)) * **dropdown-item:** Provides accessible label when href is not parsed ([#7316](#7316)) ([966b83d](966b83d)) * **flow:** Call setFocus() on back button click ([#7285](#7285)) ([9102aa4](9102aa4)) * **input-date-picker:** Provides placeholder text context for AT users ([#7320](#7320)) ([31e0ba2](31e0ba2)) * **input-date-picker:** Reset active date picker date after closing ([#7219](#7219)) ([91b2a1b](91b2a1b)) * **input, input-number:** No longer removes trailing decimal separator ([#7159](#7159)) ([01535cf](01535cf)) * **link:** Adds outline-offset to avoid overlapping with text. ([#7342](#7342)) ([c30db4e](c30db4e)) * **list:** Changing filterText property will now update filtered items ([#7133](#7133)) ([a9c0bce](a9c0bce)) * **list:** Fix keyboard navigation after a listItem's disabled or closed property changes ([#7275](#7275)) ([91d28eb](91d28eb)) * **list:** Fix keyboard navigation when filterEnabled is true ([#7385](#7385)) ([41a2e42](41a2e42)) * **menu-item:** Prevent duplicate border in nested vertical menu-items ([#7387](#7387)) ([186a738](186a738)) * **panel:** Remove double border styling when content isn't provided ([#7368](#7368)) ([91a0610](91a0610)) * Remove style modifying all host components with hidden attribute ([#7346](#7346)) ([3103e2f](3103e2f)) * **scrim:** Update loader scale on resize of component. ([#7419](#7419)) ([24e7f70](24e7f70)) * **slider:** Prevent excessive tick rendering ([#7421](#7421)) ([c799409](c799409)) * **switch:** Fix for focus outline style in certain cases ([#7414](#7414)) ([217324f](217324f)) * **tab-title:** Add full focus outline to closable tab button in high contrast mode ([#7272](#7272)) ([d812d17](d812d17)) * **tooltip:** Avoid extra before open/close event emitting ([#7422](#7422)) ([dbb6818](dbb6818)) * **tooltip:** Deprecate the label property due to the description coming from the component's content ([#7247](#7247)) ([7934d75](7934d75)) * **tooltip:** Emits `close` and `beforeClose` events when container is set to `display:none` ([#7258](#7258)) ([60a4683](60a4683)) * **tooltip:** Ensure --calcite-app-z-index-tooltip is applied ([#7345](#7345)) ([a9a7072](a9a7072)) </details> <details><summary>@esri/calcite-components-react: 1.5.0</summary> ## [1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@1.4.3...@esri/calcite-components-react@1.5.0) (2023-08-03) ### Features * Automatically import and define Calcite Components when importing their React wrapper ([#7185](#7185)) ([bf0ff67](bf0ff67)) ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^1.5.0-next.38 to ^1.5.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Elijbet added 2 commits June 16, 2023 16:57

refactor(accordion, accordion-item): as an outdated pattern

d23b4ee

Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…

e47234f

…dion-item

github-actions Bot assigned Elijbet Jun 17, 2023

Elijbet changed the title ~~Elijbet/6038 refactor get element prop accordion item~~ refactor(accordion, accordion-item): as an outdated pattern Jun 17, 2023

Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…

2bb02a3

…dion-item

github-actions Bot added the refactor Issues tied to code that needs to be significantly reworked. label Jun 17, 2023

driskull reviewed Jun 17, 2023

View reviewed changes

Elijbet added 3 commits June 17, 2023 01:45

refactoring and renaming

f0b2834

fix failing tests

5751aec

for stability, refactor test to check for prop instead of the attribu…

9af21d2

…te and correct location of the attributes

Elijbet marked this pull request as ready for review June 20, 2023 07:01

Elijbet requested a review from a team as a code owner June 20, 2023 07:01

Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jun 20, 2023

driskull previously requested changes Jun 20, 2023

View reviewed changes

Elijbet changed the title ~~refactor(accordion, accordion-item): as an outdated pattern~~ refactor(accordion, accordion-item): refactor getElementProp as an outdated pattern and get props set by parent instead Jun 20, 2023

jcfranco reviewed Jun 20, 2023

View reviewed changes

Comment thread packages/calcite-components/src/components/accordion/accordion.tsx Outdated

Comment thread packages/calcite-components/src/components/accordion/accordion.tsx Outdated

Elijbet added 2 commits June 20, 2023 18:10

refactor redundant items prop, owning accordion sets itself as parent…

4eea4b5

… on children, cleanup

Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…

bb4f0e0

…dion-item

driskull reviewed Jun 21, 2023

View reviewed changes

Comment thread packages/calcite-components/src/components/accordion/accordion.tsx Outdated

Comment thread packages/calcite-components/src/components/accordion/accordion.tsx Outdated

Elijbet added 4 commits June 22, 2023 18:22

Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…

516057a

…dion-item

cleanup

cd18c9b

Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…

cad8ffa

…dion-item

expand mutationObserver function to retrieve accordion item being add…

4644b25

…ed and it's position, which is now passed in as an argument instead of being an internal prop

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 24, 2023

jcfranco reviewed Jun 26, 2023

View reviewed changes

Comment thread packages/calcite-components/src/components/accordion/accordion.e2e.ts Outdated

Comment thread packages/calcite-components/src/components/accordion/accordion.tsx Outdated

Elijbet changed the title ~~refactor(accordion, accordion-item): refactor getElementProp as an outdated pattern and get props set by parent instead~~ fix(accordion, accordion-item): refactor getElementProp as an outdated pattern and get props set by parent instead Jun 26, 2023

Elijbet added 2 commits June 26, 2023 09:55

Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…

b90f041

…dion-item

accordion properties can be set as an attribute as well as a prop tes…

5548bff

…t coverage for #6199

simplify by updating accordion items by their order in the DOM, remov…

03ff0bc

…e the use of position

github-actions Bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jun 26, 2023

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 26, 2023

benelan reviewed Jun 26, 2023

View reviewed changes

clean up logic that relates to tracking addedItem as no longer necessary

bd1f09d

Elijbet changed the title ~~fix(accordion, accordion-item): refactor getElementProp as an outdated pattern and get props set by parent instead~~ fix(accordion, accordion-item): properties icon-position, icon-type, selection-mode, or scale can now be set as props or attributes Jun 26, 2023

benelan reviewed Jun 26, 2023

View reviewed changes

jcfranco approved these changes Jul 3, 2023

View reviewed changes

Elijbet added 2 commits July 7, 2023 15:21

Merge branch 'main' into elijbet/6038-refactor-getElementProp-accordi…

52c5b1b

…on-item

refactors and cleanup

e041429

Elijbet removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jul 7, 2023

Elijbet added 4 commits July 7, 2023 16:57

simplify updateAccordionItems to remove the use of the private accord…

3390921

…ionItems

Merge branch 'main' into elijbet/6038-refactor-getElementProp-accordi…

827c6dd

…on-item

test for prop reflects

025912e

Merge branch 'main' into elijbet/6038-refactor-getElementProp-accordi…

27638db

…on-item

Elijbet added the skip visual snapshots Pull requests that do not need visual regression testing. label Jul 8, 2023

Elijbet merged commit 2b09aba into main Jul 8, 2023

Elijbet deleted the elijbet/6038-refactor-getElementProp-accordion-item branch July 8, 2023 02:08

github-actions Bot added this to the 2023 July Priorities milestone Jul 8, 2023

calcite-admin mentioned this pull request Jul 8, 2023

chore: release latest #7261

Merged

Elijbet mentioned this pull request Jul 8, 2023

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

Closed

		@@ -98,10 +128,6 @@ export class AccordionItem implements ConditionalSlotComponent {

		connectedCallback(): void {
		this.parent = this.el.parentElement as HTMLCalciteAccordionElement;

Conversation

Elijbet commented Jun 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcfranco commented Jun 26, 2023

Uh oh!

benelan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Elijbet Jun 26, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcfranco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Elijbet commented Jun 17, 2023 •

edited

Loading

Elijbet Jun 26, 2023 •

edited

Loading