Removed interval polling.

[justinformentin]

1. Used ref for element animations
   Removed getElementById element grabbing as a ref can be used for this, and it eliminates the need to maintain the variable id attribute on the child elements.

2. Moved opening state update to individual list components

3. Removed interval polling
   The interval polling and possible change to a mutation observer is not ideal for performance. In both instances, every ListItem re-renders every time you open or close one item.

For example, in the demo app, every click was causing 400 re-renders, even though there are only 200 ListItems. With changes in this PR, every click is 1 re-render.

In this case, not only is there northing wrong with having an event listener per component, it's better for performance than the previously mentioned options, if for no other reason than just reducing re-renders.

The clickHandler now contains the logic that opens or closes the accordion, and the end result doesn't need to clean anything up with a useEffect, it doesn't need to make any decisions based on parentElements, and doesn't rely on the existence of id attributes to determine what was clicked.

I know this works differently from the existing implementation - just wanted to offer some other ways to do it with explanations.

[justinformentin]

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.

[justinformentin]

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

[ShivamJoker]

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

[justinformentin]

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

[justinformentin]

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.

[ShivamJoker]

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

[justinformentin]

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.

[justinformentin]

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.

[ShivamJoker]

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

[justinformentin]

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

[justinformentin]

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

[ShivamJoker]

After using memo all the component doesn't rerender (4 items opened, only 4 rerender)

[ShivamJoker]

Please check the new updated code

…

On 22-Feb-2022, at 9:24 PM, Justin Formentin ***@***.***> wrote: @justinformentin commented on this pull request. In src/index.tsx <#3 (comment)>: > 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"> 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: Click event fired Interval function ticking Document.getElementById Element animate() 400 re-renders With this PR its: Click event fired Element animate() 1 re-render — Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFVA4NVAX4K2N255IZWFNVLU4OWVDANCNFSM5O74HTGQ>. You are receiving this because you commented.