feat(Accordion): new component #1310
Conversation
marcinsawicki
changed the title
|
|
||
| #### Example implementation | ||
|
|
||
| ```jsx |
There was a problem hiding this comment.
Please add an import path for this example
| top: 20px; | ||
| right: 20px; | ||
| transition: inherit; | ||
| z-index: 0; |
There was a problem hiding this comment.
do we need to set it to 0 here?
There was a problem hiding this comment.
I will change that for pointer-events
| } | ||
|
|
||
| &__label { | ||
| z-index: 1; |
There was a problem hiding this comment.
do we need this z-index here?
There was a problem hiding this comment.
I will change that for pointer-events
| className | ||
| ); | ||
|
|
||
| const handleStateChange = (state: boolean) => { |
There was a problem hiding this comment.
I'd suggest avoiding "empty words" like "state", "value" etc, because those words alone are not providing any context to the function/variable name - because the word "state" could mean anything. So I'd suggest using in this case the word "visibility": handleVisibilityChange, newVisibilityValue.
|
|
||
| return () => resizeObserver.disconnect(); | ||
| } | ||
| }, [contentRef]); |
There was a problem hiding this comment.
Are you sure this effect works as intended? The contentRef is a reference so it's mutable and the useEffect between the renders will not detect its changes.
| if (isOpen) { | ||
| setIsOpenMounted(true); | ||
| setIsClosedVisible(false); | ||
|
|
||
| requestAnimationFrame(() => setIsOpenVisible(true)); | ||
|
|
||
| if (closeRef) { | ||
| closeRef.addEventListener( | ||
| 'transitionend', | ||
| () => { | ||
| setIsClosedMounted(false); | ||
| }, | ||
| { once: true } | ||
| ); | ||
| } | ||
| } else { | ||
| setIsClosedMounted(true); | ||
| setIsOpenVisible(false); | ||
|
|
||
| requestAnimationFrame(() => setIsClosedVisible(true)); | ||
|
|
||
| if (openRef) { | ||
| openRef.addEventListener( | ||
| 'transitionend', | ||
| () => { | ||
| setIsOpenMounted(false); | ||
| }, | ||
| { once: true } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Both sides of the if statement are nearly identical and can be simplified using the ternary operator and the isOpen value itself
| () => { | ||
| setIsOpenMounted(false); | ||
| }, | ||
| { once: true } |
There was a problem hiding this comment.
This option removes the event listener only if it was invoked, but if not - we need to clear ourselves.
|
|
||
| return () => resizeObserver.disconnect(); | ||
| } | ||
| }, [multilineRef]); |
There was a problem hiding this comment.
references in react a mutable, so the effect cannot detect the ref changes between the renders
| React.useEffect(() => { | ||
| const hasIOSupport = !!window.ResizeObserver; | ||
|
|
||
| if (multilineRef.current && hasIOSupport) { | ||
| const resizeObserver = new ResizeObserver((entries) => { | ||
| const entry = entries[0]; | ||
| if (!entry) return; | ||
|
|
||
| const newSize = entry.contentRect.height; | ||
|
|
||
| if (previousMultilineSizeRef.current !== newSize) { | ||
| setSize(newSize); | ||
| previousMultilineSizeRef.current = newSize; | ||
| } | ||
| }); | ||
|
|
||
| resizeObserver.observe(multilineRef.current); | ||
|
|
||
| return () => resizeObserver.disconnect(); | ||
| } | ||
| }, [multilineRef]); |
There was a problem hiding this comment.
This code is nearly identical to the effect from Accordion.tsx file.
|
Caught a bug with picker inside accordion:
|
Resolves: #1206
Description
Accordion new component.
Storybook
https://feature-accordion-new-component--613a8e945a5665003a05113b.chromatic.com/?path=/story/components-accordion--examples
Checklist
Obligatory: