Skip to content
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

fix(accordion, accordion-item): icon-position, icon-type, selection-mode and scale can now be set as props or attributes #7191

Merged
merged 22 commits into from
Jul 8, 2023
Merged

fix(accordion, accordion-item): icon-position, icon-type, selection-mode and scale can now be set as props or attributes #7191

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d23b4ee
refactor(accordion, accordion-item): as an outdated pattern
Elijbet Jun 16, 2023
e47234f
Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…
Elijbet Jun 16, 2023
2bb02a3
Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…
Elijbet Jun 17, 2023
f0b2834
refactoring and renaming
Elijbet Jun 17, 2023
5751aec
fix failing tests
Elijbet Jun 20, 2023
9af21d2
for stability, refactor test to check for prop instead of the attribu…
Elijbet Jun 20, 2023
4eea4b5
refactor redundant items prop, owning accordion sets itself as parent…
Elijbet Jun 21, 2023
bb4f0e0
Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…
Elijbet Jun 21, 2023
516057a
Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…
Elijbet Jun 23, 2023
cd18c9b
cleanup
Elijbet Jun 23, 2023
cad8ffa
Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…
Elijbet Jun 23, 2023
4644b25
expand mutationObserver function to retrieve accordion item being add…
Elijbet Jun 24, 2023
b90f041
Merge branch 'master' into elijbet/6038-refactor-getElementProp-accor…
Elijbet Jun 26, 2023
5548bff
accordion properties can be set as an attribute as well as a prop tes…
Elijbet Jun 26, 2023
03ff0bc
simplify by updating accordion items by their order in the DOM, remov…
Elijbet Jun 26, 2023
bd1f09d
clean up logic that relates to tracking addedItem as no longer necessary
Elijbet Jun 26, 2023
52c5b1b
Merge branch 'main' into elijbet/6038-refactor-getElementProp-accordi…
Elijbet Jul 7, 2023
e041429
refactors and cleanup
Elijbet Jul 7, 2023
3390921
simplify updateAccordionItems to remove the use of the private accord…
Elijbet Jul 7, 2023
827c6dd
Merge branch 'main' into elijbet/6038-refactor-getElementProp-accordi…
Elijbet Jul 7, 2023
025912e
test for prop reflects
Elijbet Jul 8, 2023
27638db
Merge branch 'main' into elijbet/6038-refactor-getElementProp-accordi…
Elijbet Jul 8, 2023
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
24 changes: 3 additions & 21 deletions packages/calcite-components/src/components/accordion/accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,7 @@ export class Accordion {
//
//--------------------------------------------------------------------------

mutationObserver = createObserver("mutation", (mutationList) => {
let addedAccordionItem: HTMLCalciteAccordionItemElement;
for (const mutation of mutationList) {
if (mutation.type === "childList") {
const addedNodes = Array.from(mutation.addedNodes);
addedAccordionItem = addedNodes.find(
(node) => node instanceof HTMLCalciteAccordionItemElement
) as HTMLCalciteAccordionItemElement;
}
}
this.updateAccordionItems(addedAccordionItem);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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).

});
mutationObserver = createObserver("mutation", () => this.updateAccordionItems());
Elijbet marked this conversation as resolved.
Show resolved Hide resolved

/** list of `accordion-item`s */
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
accordionItems: HTMLCalciteAccordionItemElement[] = [];
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -153,15 +142,8 @@ export class Accordion {
//
//--------------------------------------------------------------------------

private updateAccordionItems = (addedAccordionItem?: HTMLCalciteAccordionItemElement): void => {
if (addedAccordionItem) {
this.accordionItems.push(addedAccordionItem);
} else {
const accordionItemsQueryArray = Array.from(
this.el.querySelectorAll("calcite-accordion-item")
);
this.accordionItems = accordionItemsQueryArray;
}
private updateAccordionItems = (): void => {
this.accordionItems = Array.from(this.el.querySelectorAll("calcite-accordion-item"));
Elijbet marked this conversation as resolved.
Show resolved Hide resolved

this.accordionItems.forEach((item) => {
const accordionItem = item;
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
Expand Down