[WIP] Feature/scatter plot basic - #6
Conversation
c605a56 to
95c2e90
Compare
brianbancroft
left a comment
brianbancroft
left a comment
There was a problem hiding this comment.
Here is a choice arrangement of words. More a comment, much less a review.
| const drawAScatter = (left, top) => { | ||
| return ( | ||
| <GlyphDot | ||
| key={Math.random()} |
There was a problem hiding this comment.
So this is more commentary, and it's more intended for @woozyking. Please don't see this as a request for change at this time. I've just since run into a relevant piece in the React docs, which might be worth bringing to light. This is certainly not a critique of the work being done at this time!
The reason why I brought up math.random() as a key being a bad idea is that it causes issues with the process of reconciliation, where React tries to determine whether a subset of a component should be updated, or whether an entire component should be rendered.
The end of the page I've linked to seems to suggest that when employing math.random() as a key, there is a much higher chance that the element in question will be rebuilt completely, rather than partially.
If that's what is intended the context of this code, I'm not sure. d3 and charts being what they are, maybe stripping down and rebuilding is exactly what we want here.
There was a problem hiding this comment.
I think both @y629wang and yourself have mixed this with the case from the last PR #4 (comment), where it did make sense and the intent was to re-render the tooltip to take advantage of the dynamically computed boundary offered by vx.
From the look of this case, there's no reason to use Math.random() as key.
| const drawAScatter = (left, top) => { | ||
| return ( | ||
| <GlyphDot | ||
| key={Math.random()} |
There was a problem hiding this comment.
I think both @y629wang and yourself have mixed this with the case from the last PR #4 (comment), where it did make sense and the intent was to re-render the tooltip to take advantage of the dynamically computed boundary offered by vx.
From the look of this case, there's no reason to use Math.random() as key.
| ) | ||
| } | ||
|
|
||
| // eslint-disable-next-line |
There was a problem hiding this comment.
There's no need to use linter disable (and frankly we should minimize the use of it), since you're trying to handle the case of unused variable but the unused variable is a positional argument of the function handler -- simply remove it:
const scatterShape = (d) => { }will do
3 participants
A basic scatterplot to serve as minimized view of
scatter pie. The plan is to combine withscatter pieto create both minimized view and expanded views.