search

IjzerenHein / react-tag-cloud

Create beautiful tag/word clouds using React ☁️
https://react-tag-cloud.stackblitz.io/
MIT License
117 stars 27 forks source link

Feat: added disableResize prop #16

Open jorgemmsilva opened 5 years ago

jorgemmsilva commented 5 years ago

Thanks for the lib, this works well!

This PR adds an optional disableResize prop.

Reason being: while using this lib on a project, onResize is being triggered mid-animation and contentRect.bounds has the wrong values, so the component gets rendered incorrectly. (line: https://github.com/IjzerenHein/react-tag-cloud/blob/master/src/TagCloud.tsx#L252)

disableResize enables users to opt out of the existing behaviour

(We can implement this in a different way if you want to discuss it)

Thanks

IjzerenHein commented 5 years ago

Hi, thanks for this contribution. I've been quite busy lately but I'll try to take a closer look at this anytime soon. Cheers, Hein

IjzerenHein commented 5 years ago

Hi, I feel that the problem that you are trying to fix here, is being solved in the wrong manner and in the wrong location. The fix you suggest simply disables resize handling for this component. It does not handle the case where you re-enable the resize behavior.

Honestly, I feel that if you want fine grained control over the size of this component then you should implement a parent div and control its size accordingly and in-sync with your animation.

I can imagine that the default resize debounce behavior if undesirable in that case. I'd rather see a prop to configure the resizeDebounceTimeout. The default of this is 100, but you can also set it to 0, to make it respond immediately. You can then control the resize trigger in the parent-div by controlling the size of the parent.

jorgemmsilva commented 5 years ago

To be fair it will do any future resizes if disableResize is changed (from true) to false.

Adding a customisable resizeDebounceTimeout sounds like a good idea, tho.

(FYI ended up forking the code and removing the resize functionality all together, so won't be making updates to this PR.)

IjzerenHein commented 5 years ago

Alright, well thanks for the heads up.