search

AnyRoad / react-json-view-lite

Lightweight Json view component for React
155 stars 14 forks source link

store shouldInitiallyExpand in ref #8

Closed dartess closed 1 year ago

dartess commented 1 year ago

Now when re-rendering, shouldInitiallyExpand is re-run. For example, I use this code:

shouldInitiallyExpand={level => level < 2}

If I expand or collapse any node, i got reset after the rerender.

It is worth putting a callback in a ref to prevent this behavior.

I understand that this can be avoided by using useCallback, or by moving the function outside. But this will require it to be typed and in general to make unnecessary movements. In addition, this is unintuitive and unfriendly for users - I took a ready-made example, adjusted it to myself and got a bug. If you do not agree to make corrections, you should at least change the example.

AnyRoad commented 1 year ago

Hi, thank you for the feedback! Yes, you are right, it is worth to change the example and explain that part clearly. I've just updated the README.

It is worth putting a callback in a ref to prevent this behavior.

If we put the shouldInitiallyExpand to ref I afraid it will be a difficult to change the shouldInitiallyExpand property to update the component. For instance, if we want to have a button to expand/collapse all child nodes with level > 1. (we can swith the shouldInitiallyExpand parameter between allExpanded, collapseAllNested functions provided by the library)

dartess commented 1 year ago

If we put the shouldInitiallyExpand to ref I afraid it will be a difficult to change the shouldInitiallyExpand property to update the component.

I think a component should not respond to a change in a prop named "shouldInitiallyExpand". This is another point that confused me.

Also I think if the prop was called, for example, shouldExpand, I would not even open an issue, I just thought "my bad" and made a fix on my side.

AnyRoad commented 1 year ago

Yes, I think the name can be misleading a little... I guess I just named it similar to another library. E.g., in the https://www.npmjs.com/package/react-json-tree it is called "shouldExpandNodeInitially". I've added more description to the README. It is probably better to rename the property in the next major version upgrade, probably 1.0.0.

AnyRoad commented 1 year ago

Hi,

Just released the 1.0.0 version, please refer to the migration guide.

Thanks, Andy.