Skip to content

Component | Scatter: MakesizeScale immutable to prevent sizeRange collisions#411

Merged
reb-dev merged 2 commits intomainfrom
fix/gallery-scatter-brush
Jul 8, 2024
Merged

Component | Scatter: MakesizeScale immutable to prevent sizeRange collisions#411
reb-dev merged 2 commits intomainfrom
fix/gallery-scatter-brush

Conversation

@reb-dev
Copy link
Copy Markdown
Contributor

@reb-dev reb-dev commented Jul 1, 2024

Summary

This PR makes Scatter's sizeScale config property immutable so that if there is a shared instance to config or config.sizeScale, the sizeRange property does not get overridden. More explanation below as it is a bit of an edge case.

Background

When trying to figure out why this scatter gallery example looks wrong (i.e. the minimap's points are too large for sizeRange: [3, 10]), I discovered that the issue comes from using a shared config object between the two charts. Since sizeScale is undefined, when it is initialized with the default value in the first component, that becomes the same instance of sizeScale for both.

This means that despite both components specifying different values for config.sizeRange, when the hidden shared reference to config.sizeScale ends up being mutated in the _updateSizeScale function, it overrides any previous domain or range that was set. For this reason I think we should use a private instance instead of manipulating config.sizeScale directly.

Expected behavior

Given the following two scatters with the same data:

const props = { x: d => d.x, y: d => d.y, size: d => d.x }

{/* Scatter 1 */}
<VisScatter {...props} sizeRange={[2, 10]}/>

{/* Scatter 2 */}
<VisScatter {...props} sizeRange={[4, 32]}/>

We expect the first to have points ranging in radius from 2-10px and the second in from 4-32px.

Before

Screenshot 2024-07-01 at 1 33 56 PM

After

Screenshot 2024-07-01 at 1 32 55 PM

@rokotyan
Copy link
Copy Markdown
Contributor

rokotyan commented Jul 2, 2024

@reb-dev This is great that you were able to find the cause of this problem! Apparently it happened when we switched from using config classes to simple objects and I completely missed it. Did you also check other components for similar initializations?

@reb-dev
Copy link
Copy Markdown
Contributor Author

reb-dev commented Jul 5, 2024

@rokotyan Yes, from what I can tell this is the only instance

@reb-dev reb-dev requested a review from lee00678 July 5, 2024 21:50
Copy link
Copy Markdown
Collaborator

@lee00678 lee00678 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@reb-dev reb-dev merged commit ad574b3 into main Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants