new(wordcloud): add @visx/wordcloud #1311
Conversation
| const [cloudWords, setCloudWords] = useState<d3Cloud.Word[]>([]); | ||
|
|
||
| useEffect(() => { | ||
| const layout = d3Cloud<Datum>(); |
There was a problem hiding this comment.
Maybe we don't have to do any of these layout thing at all if width === 0 || height === 0?
if (width === 0 || height === 0) {
return;
}
There was a problem hiding this comment.
wow thanks for the great contribution @craciuncezar 🙌 !!!
this looks really great, I had just a few comments which hopefully won't be too tricky to address. excited for this to land 🔜 !
williaster
changed the title
| }: WordcloudConfig<Datum>) { | ||
| const [cloudWords, setCloudWords] = useState<d3Cloud.Word[]>([]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This hook uses a useEffcect, otherwise the d3 wordcloud would recreate its view every render.
The example that uses this hook, passes as a prop an inline function declaration. I would have thought that mean a new reference in the deps array every render, causing the wordcloud to constantly change if the parent component rerenders.
Does that not happen? Or was this accounted for somehow?
There was a problem hiding this comment.
^I had a similar question, I think it would for sure have called the wordcloud layout for each render with the inline function. In the example specifically I think that would only have happened with a size change of the parent, but probably best to implement the example with perf in mind.
There was a problem hiding this comment.
purely for curiousity sake, do you think theres a way to change it inside the hook not to be concerned about constant rerender? i think this is unique across most of the visx packages right?
There was a problem hiding this comment.
I'm open to suggestions however I don't see how we can avoid this 🤔, I know it's not ideal but, as usual in React, it's up to the consumer to memoize or wrap fn in useCallback.
There was a problem hiding this comment.
Yeah I think unfortunately there's probably not a clean way to remove this, as @craciuncezar said it's common in React for memoization logic to be pushed to the consumer. We have this same requirement in other layout code in visx, I would guess that many consumers don't optimize their usage, but if they don't see perf issues it's probably okay (wordcloud perf may present more of an issue since it's a more complex algorithm)
There was a problem hiding this comment.
Woohoo looking great @craciuncezar ! Thanks for all of the iteration on this.
I had 2 more minor comments but otherwise lgtm!
|
New happo visual example looks good 📈 ✨ |
|
I don't think we've released new packages with CI before so it might fail but ... yolo |
I'm glad I could contribute to this awesome lib, thanks for all the work 🚀 |
|
🎉 This PR is included in version |
🚀 Enhancements