Skip to content
This repository has been archived by the owner on Dec 2, 2021. It is now read-only.

Feat: added disableResize prop #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jorgemmsilva
Copy link

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
Copy link
Owner

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
Copy link
Owner

IjzerenHein commented Jul 22, 2019

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
Copy link
Author

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
Copy link
Owner

Alright, well thanks for the heads up.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants