Hookify all components (WIP)

[TimVanMourik]

This is an attempt to rewrite all classes with React hooks.

While rewriting, I got in trouble with the value variable. It's basically containing the entire state of the component, but is managed outside of the React state management. The current rewrite tries to unify this.

The current status of the rewrite is

• [x] has all React.Components removed and replaced by function components with hooks
• [x] rewrites the state management in terms of React hooks, by removing the manual setting and freezeing.
  • [x] panning and zooming have been updated
  • [ ] touch interactions still need to be rewritten
  • [x] ideally, the getValue function returns the current state wrapped as the same value as is currently the case, to retain the existing API.
• [x] I broke the autopan, and still need to fix it

This is still Work In Progress, do not merge yet.

[chrvadala]

This is a great feature and probably will require a new major release 👍 Thanks for your work. Feel free to ask me help 😎

[TimVanMourik]

Hi @chrvadala (and other potentially interested folks), just a check-in update:

• I finished the bulk of the work: the viewer is completely hookified now. Along the way, I had to refactor quite a large portion of the repo though. A bit too much to explain here, so I must invite you to look at the code. I had to get rid of the way the value is dealt with, because it's rather antithetical to the hooks way of doing things.

• Functions are now much more explicit (but also more verbose) in what variables they pass on. Sometimes that's a bit clunky and it would be good to simplify it.

• There are some minor things I haven't updated or tested yet, like the touch events.

• Along the way, I refactored some parts of the code, like the autoPan. I believe the current implementation is a lot more intuitive by getting rid of the pointer variable and reworking it in a hover event on the borders.

I would very much welcome a look on this and would love to hear your opinion on the changes. As you mentioned before, this most certainly requires a new major release once it's all done.

[chrvadala]

Thanks for your huge work. You changed a lot of files, so a review might require time. I'll provide you a feedback as soon as possible.

[chrvadala]

Hi @TimVanMourik , I had the opportunity to review your changes, so here my comments:

• You split the value prop into multiple objects. The idea behind the previous architecture is having an object that is the single source of truth for a state. After splitting, you lost the capability to serialise and de-serialise a single state. This feature is quite important because it allows us advanced scenarios like this one https://github.com/chrvadala/react-svg-pan-zoom/blob/master/examples/controlled-component-advanced-usage/src/App.js
• Today this library provides a ControlledViewer and a UncontrolledViewer in a way that follows the React Input Text pattern. Except for some cached values the ControlledViewer (https://github.com/chrvadala/react-svg-pan-zoom/blob/master/src/viewer.js) is a stateless component, so it accepts a value and a callback onChangeValue like any React controlled components (https://reactjs.org/docs/forms.html#controlled-components). I think we can't loose this capability.
• You changed a lot of things into /features folder. I understand why you did it, but I think that is better to keep these files unchanged, anyway If you use the approach based on value object you don't need so much changes here.

Finally, I'd like to provide a new library version based on React Hooks, but I need to keep any available features in order to avoid breaking changes. I think that we can work together to reach this goal. On a different branch I started to write some unit tests, because I'd like to release a new major versione with higher code coverage. Please feel free to contact me if you have any question

[TimVanMourik]

Hi @chrvadala, Thanks for your time to look into the PR.  I realise I made a massive update without first discussing serious API changes. Communication for open source projects can be a bit slow, so I decided just to go ahead and see what you'd think of the result. So thanks again for the feedback!

• Indeed I split up the value prop. Within a hooks context it is really quite difficult to have a single value like in the current implementation... But I recognise the value of that implementation as integral part of the API. Now the way that is both hooks compatible and single value compatible is redux state management and useReducer. That allows you to serialize the state quite easily.
• Also the onValueChange callback is pretty easily accomplished with redux state management. The purely declarative way is slightly trickier. I would have to think about that.
• Yeah, sorry about that. I felt the current way of handling the features is a bit obscure. It is somewhat untransparent to see what components are changed by each function. And passing on long lists of parameters that are just part of the state isn't all too readable. Now there's a way to separate actions and state changes... redux! 😀

Ok, this sounds like I'm a huge redux fan. Which is partially correct, but in this case I just genuinely think it might resolve many problems. I'm going to try and prepare some proof of principle and maybe I can convince you it's a good idea :) If not, no worries, then I'm sure it was a good exercise for me.

[chrvadala]

Hei man, you did a lot of work here https://github.com/chrvadala/react-svg-pan-zoom/pull/162. I appreciate it but, as suggested in my previous comment, you don't need to refactor all these stuffs to obtain a hooked version of the viewer. I agree that reducers are a great thing. I built on top of Redux this other library https://github.com/cvdlab/react-planner, so I know very well this library, but tough I like this pattern I think that is better to keep this library as much as decoupled from other libraries.

Man, I'd like to give you the right way in order to avoid you to make something that I can't merge. I did a brief analyse of what works is needed in order to obtain a hooked version of this library. Hope that this list might help you:

• [ ] Create hook useRandomUID that replaces RandomUID component
• [ ] Replace HOC with useRandomUID hook from here src/ui/border-gradient.js
• [ ] Replace HOC with useRandomUID hook from here src/ui-miniature/miniature-mask.jsx
• [ ] Hookify this component src/ui-toolbar/toolbar-button.jsx
• [ ] Hookify this component src/uncontrolled-viewer.js
• [ ] Hookify this component src/viewer.js

From my side I'm working in order to make stronger the rest, adding some unit tests. I'm pushing my code on this PR https://github.com/chrvadala/react-svg-pan-zoom/pull/164 .

Feel free to ask me for any question. Thanks again for your contribution.