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

Jump to bottom

Removed interval polling. #3

Closed
wants to merge 1 commit into from
Closed

Removed interval polling. #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 0 additions & 29 deletions src/ListItem.tsx
View file
Open in desktop

This file was deleted.

109 changes: 51 additions & 58 deletions src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React, { MouseEvent, useEffect, useState } from "react";
import ListItem from "./ListItem";
import React, { MouseEvent, useEffect, useState, useRef } from "react";
import "./style.css";

interface AccordionItem {
Expand All @@ -13,72 +12,66 @@ interface AccorionProps {
[rest: string | number | symbol]: unknown;
}

let interval: number;
interface ListItemProps {
SummaryComponent: React.ElementType;
DetailComponent: React.ElementType;
id: string | number;
}

const Accordion = ({ items, ...rest }: AccorionProps) => {
const [opened, setOpened] = useState<Record<string, boolean>>({});
const ListItem = ({
id,
SummaryComponent,
DetailComponent,
...rest
}: ListItemProps) => {
const contentItem = useRef<HTMLDivElement>(null);
const [opened, setOpened] = useState<boolean>(false);
Copy link
Owner

Choose a reason for hiding this comment

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

Also this will create a new state in every LI so I am not sure about the memory usage.

Copy link
Author

Choose a reason for hiding this comment

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

The memory usage actually seems lower in my initial tests. I'll post memory snapshot data.

Copy link
Author

Choose a reason for hiding this comment

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

Changes in the PR memory - https://imgur.com/a/J1ieNTs
Current memory - https://imgur.com/a/BqlC0NI


const clickHandler = (e: MouseEvent): void => {
let element = e.target as HTMLElement;

if (element.parentElement?.tagName === "LI") {
element = element.parentElement;
}

if (element.tagName !== "LI") return;

const id = element.getAttribute("id");

if (!id) return;

const isOpen = !!opened[id];

if (isOpen) {
const contentItem = document.getElementById(`acc-item-${id}`);

if (!contentItem) return;

contentItem
.animate(
{ maxHeight: 0, opacity: 0 },
{ duration: 100, easing: "ease-out" }
)
.finished.then(() => {
setOpened((prv) => ({ ...prv, [id]: false }));
});
return;
}

setOpened((prv) => ({ ...prv, [id]: true }));

// listen for DOM to be added
interval = setInterval(() => {
const contentItem = document.getElementById(`acc-item-${id}`);
if (!contentItem) return;
if (contentItem?.scrollHeight) {
const scrollHeight = contentItem.scrollHeight;

contentItem.animate(
{ maxHeight: `${scrollHeight}px`, opacity: 1 },
{ duration: 100, easing: "ease-in", fill: "forwards" }
);
clearInterval(interval);
if (opened && contentItem.current) {
const element = e.target as HTMLElement;
const clickedInside = contentItem.current.contains(element);
Copy link
Author

Choose a reason for hiding this comment

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

Maintains functionality of only opening and closing the accordion when the main accordion header is clicked, so user passed props and behavior, like the demo app's onClick, can be fired, and the accordion doesn't close.

if (!clickedInside) {
contentItem.current
.animate(
{ maxHeight: 0, opacity: 0 },
{ duration: 100, easing: "ease-out" }
)
.finished.then(() => setOpened(false));
}
}, 5);
} else {
setOpened(true);
}
};

useEffect(() => {
// remove on unmount
return clearInterval(interval);
}, []);
if (opened && contentItem && contentItem.current?.scrollHeight) {
Copy link
Author

Choose a reason for hiding this comment

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

Logic placed in useEffect so that setOpened(true) can cause a re-render and contentItem can exist before trying to animate it.

const scrollHeight = contentItem.current.scrollHeight;
contentItem.current.animate(
{ maxHeight: `${scrollHeight}px`, opacity: 1 },
{ duration: 100, easing: "ease-in", fill: "forwards" }
);
}
}, [opened]);

return (
<ul onClick={clickHandler}>
{items.map(({ id, ...data }) => (
<ListItem id={id} {...data} key={id} isOpen={opened[id]} {...rest} />
))}
</ul>
<li onClick={clickHandler} className="acc-item">
Copy link
Owner

Choose a reason for hiding this comment

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

Don't you think adding a click handler on every element will cause some performance issues?

Copy link
Author

Choose a reason for hiding this comment

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

No, I'll post screenshots of the performance flame graph, in my initial tests it looks like this is far more performant.

Adding event listeners by themselves is not a performance issue. Having many events firing could be a performance issue, but in this case you have only one event at a time. The main difference is can be seen just by breaking down what's happening within the app when you click on an accordion.

Currently its:

  1. Click event fired
  2. Interval function ticking
  3. Document.getElementById
  4. Element animate()
  5. 400 re-renders

With this PR its:

  1. Click event fired
  2. Element animate()
  3. 1 re-render

Copy link
Author

Choose a reason for hiding this comment

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

Changes in the PR - https://imgur.com/a/VEtWPyY
Current code - https://imgur.com/a/YLvoivT

There is so much less going on in the first photo - even though every element has an event listener, and every element will update their state, it only does it one at a time. Every click is one event, one state update, one re-render. That's why it's so much more performant.

Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure if you just use state individually, how will you close other accordions?
I've added it as new feature.

Thanks for all the effort you put

Copy link
Author

Choose a reason for hiding this comment

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

No problem!

As for closing other Accordions, what I'd probably do is something to share the "open/close" logic, maybe as a hook, so when one item opens, all the other items are aware and automatically close.

For example, convert that logic to a const [opened, setOpened] = useToggleState(false)

and inside the useToggleState, do what you did initially, make the opened state the id of the element that is open.

Then your isOpen will just be open === element.id.

This could also be done by firing an event, but that's not ideal.

Copy link
Author

Choose a reason for hiding this comment

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

Also, just wanted to mention, using a MutationObserver will have the same performance issues that using the interval has - it's slightly better since you're removing the setInterval, but the MutationObserver will still include the hundreds of re-renders per click if the end result is that the parent element holds the toggle state.

<SummaryComponent {...rest} isOpen={opened} />
{opened && (
<div className="acc-content" ref={contentItem}>
<DetailComponent {...rest} isOpen={opened} />
</div>
)}
</li>
);
};

const Accordion = ({ items, ...rest }: AccorionProps) => (
<ul>
{items.map(({ id, ...data }) => (
<ListItem id={id} {...data} key={id} {...rest} />
))}
</ul>
);

export default Accordion;