Some optimisations on the component #7

s-yadav · 2020-08-05T14:19:57Z

Hey @dahliacreative. While searching for micro react component for similar use case found your library. Great work. 👍

This is a draft PR with few improvements. Please let me know what do you think. I will add the test cases afterwards.

Changes:

Remove children when it's hidden. Improves React + DOM performance.
Add prop to keep the child when closed.
Use onTransitionEnd instead of setTimeout.
Forward remaining props on root element.

Also, I feel collapsable should be collapsible. Is this intentional? Should we keep a different name for it? I see there is already a library with react-collapsible name on npm.

Use this to ignore formatting changes.
https://github.com/dahliacreative/react-collapsable/pull/7/files?w=1

- Add prop to keep the child when closed. - Use onTransitionEnd instead of setTimeout. - Forward remaining props on root element

- @emotion-core is not required as peer dependency

dahliacreative · 2020-08-20T15:26:57Z

src/index.js

+    speed: 0,
+  });
+
+  const [effectiveChildren, setChildren] = useState(children);


Storing the children in state seems a bit over the top, could we not just add another option to state like showChildren: boolean and combine with keepChildrenOnClose to conditionally add the children in the JSX?

Also, adding / removing children when opening / closing is causing more work for react as it's having to add / remove them from the DOM, so do you have any evidence of performance gains?

Storing the children in state seems a bit over the top, could we not just add another option to state like showChildren: boolean and combine with keepChildrenOnClose to conditionally add the children in the JSX?

I guess adding showChildren will be just an extra prop. As most of the time lib user will pass isOpen value on showChildren. Also, it can't be handled outside as the effectiveChildren is set to null only after transition is finished. so then you will have to pass an onTransitionComplete prop, or onOpen/onClose prop to parent.

Also, adding / removing children when opening / closing is causing more work for react as it's having to add / remove them from the DOM, so do you have any evidence of performance gains?

So, this comes down to this question.
-> Do you want to pretender everything so when a user performs an action the transition is quick.?
-> Do you want to delegate the offscreen work when the user performs an action.?

On the first approach, you are paying the cost of rendering upfront for some content user may never see, probably on page load.
Plus as other things are going during page rendering, you are also adding some of the rendering time for an offscreen content.
This is valid on the update as well, why we need to do reconciliation (VDOM diffing) on offscreen content.

And on the second approach as onClick, you are doing relatively less work by shifting your work to onClick which will not be noticed. I feel this is a valid tradeoff to improve onscreen content performance. Sometime you may need a little bit of tinkering, but that you can do with keepChildrenOnClose prop.

My main thoughts on this is accessibility and not hiding content from screen readers. Having a prop puts this in the hands of the developer, but I think having the children when closed should be the default.

As for storing children in state, we can simply use a value stored in state to conditionally render the children and isOpen will override it.

I think storing children in state and removing them constantly would be inefficient and there is something we can do with a simple Boolean instead.

My main thoughts on this is accessibility and not hiding content from screen readers. Having a prop puts this in the hands of the developer, but I think having the children when closed should be the default.

display: none and visibility: hidden elements are not accessible on-screen reader as well. In Ideal situation content wise we see should be similar to what screen reader reads. Providing them two versions is anti-pattern for accessibility. Exceptions are visual contents like img, icons etc. But textual content-wise it should be same for screen reader and normal user.

I think storing children in state and removing them constantly would be inefficient and there is something we can do with a simple Boolean instead.

Sure, I will update it boolean based.

dahliacreative · 2020-08-20T15:30:13Z

src/index.js

-}
+  return (
+    <div
+      {...restProps}


This could lead to invalid props being passed down. I'm not sure it's needed as for custom styling you can just wrap it in a custom component which has it's own styling etc..

My opinion:
This can be mentioned in the document. Forwarding non-component specific props to the root element can avoid the extra containers in dom elements if possible.

The actual use cases can definitely be solved other way as well. So I don't have a strong opinion on this. I will revert it back.

Yeah I feel allowing for users to add their own class name to the wrapper would suffice.

dahliacreative · 2020-08-20T15:30:54Z

src/index.js

+import PropTypes from 'prop-types';

-const Collapsable = ({
+const Collapsible = ({


Collapsable was intentional, you can spell it both ways and there is already a react-collapsible on NPM.

I will change this back.

package.json

- Use boolean flag instead of storing children in state. - Remove spread props and rename the name to react-collapsable

dahliacreative · 2020-09-24T10:24:15Z

One other thing, can we update the read me to please? :)

s-yadav · 2020-10-07T12:02:36Z

@dahliacreative Sorry missed this. Sure will update.

dahliacreative · 2020-09-24T10:17:19Z

src/index.js

        speed: 0
    })

+    const [keepChildren, toggleChildren] = useState(isOpen)


can this just go into the state object above?

dahliacreative · 2020-09-24T10:20:55Z

src/index.js

+        if (!isOpen) {
+            content.current.style.visibility = 'hidden'
+
+            if (!keepChildrenOnClose) {


I would remove this and just add it to the conditional below {(keepChildren || keepChildrenOnClose) && children}
Perhaps also change keepChildren to something like displayChildren or shouldDisplayChildren feels a bit clearer.

dahliacreative · 2020-09-24T10:21:29Z

src/index.js

                    ? maxAnimationDuration
                    : time
            content.current.style.visibility = 'visible'
+            toggleChildren(true)


Again if we put this in the state object you can just set this below.

- Remove children when its hidden. Improves React + DOM performance.

2e64fac

- Add prop to keep the child when closed. - Use onTransitionEnd instead of setTimeout. - Forward remaining props on root element

s-yadav force-pushed the optimizations branch from 33abbfb to 2e64fac Compare August 5, 2020 14:24

- Use plugin transform runtime to avoid having babel runtime in bundle.

8339f45

- @emotion-core is not required as peer dependency

dahliacreative reviewed Aug 20, 2020

View reviewed changes

s-yadav force-pushed the optimizations branch from d2ff76b to 1585383 Compare September 16, 2020 12:46

- Add react-dom as peer dependency and dev dependency

abc4379

- Use boolean flag instead of storing children in state. - Remove spread props and rename the name to react-collapsable

s-yadav force-pushed the optimizations branch from 1585383 to abc4379 Compare September 17, 2020 06:35

dahliacreative reviewed Nov 4, 2020

View reviewed changes

Some optimisations on the component #7

Are you sure you want to change the base?

Some optimisations on the component #7

Uh oh!

Conversation

s-yadav commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-yadav Aug 21, 2020 • 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!

s-yadav Sep 16, 2020 • 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!

s-yadav Aug 21, 2020 • 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!

Uh oh!

dahliacreative commented Sep 24, 2020

Uh oh!

s-yadav commented Oct 7, 2020

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

s-yadav commented Aug 5, 2020 •

edited

Loading

s-yadav Aug 21, 2020 •

edited

Loading

s-yadav Sep 16, 2020 •

edited

Loading

s-yadav Aug 21, 2020 •

edited

Loading