Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove line element from scatter controller #10439

Merged
merged 6 commits into from
Aug 1, 2022

Conversation

thabarbados
Copy link
Contributor

Make scatter chart not require the lineElement #10242

You can check the build with changes here

Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Some weird animation behaviour. If you enable the showLine prop for Scatter Dataset 1 it will disable the animations of Scatter Dataset 2. This dataset will pop in with showLine true and false

src/core/core.controller.js Outdated Show resolved Hide resolved
src/controllers/controller.scatter.js Outdated Show resolved Hide resolved
src/controllers/controller.scatter.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

I think it would be better to not make these helper functions public, this means they can not be changed easily since people might rely on them. But as long as we keep them private they can be changed more easily in the future. And we can always make them public but making them private needs to wait for a major release

Also if you make them private there is no type declaration needed for them.

@thabarbados
Copy link
Contributor Author

@LeeLenaleee, thanks for your review. I made helper functions private.

@thabarbados thabarbados requested a review from LeeLenaleee June 29, 2022 19:25
@thabarbados
Copy link
Contributor Author

Hey folks! Is there anything else I need to do to approve this PR?

@LeeLenaleee
Copy link
Collaborator

I'm sorry, had little time because my graduation internship was comming to an end, I will look a or it tonight, should have time then

Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Noticed that when you set showLine to true in the options object itself it will throw an error:
https://codesandbox.io/s/peaceful-forest-ttcudi?file=/src/components/ScatterChart.ts

src/controllers/controller.scatter.js Outdated Show resolved Hide resolved
src/controllers/controller.scatter.js Show resolved Hide resolved
@thabarbados thabarbados requested a review from LeeLenaleee July 12, 2022 13:22
LeeLenaleee
LeeLenaleee previously approved these changes Jul 19, 2022
@etimberg etimberg requested a review from kurkle July 27, 2022 12:32
etimberg
etimberg previously approved these changes Jul 27, 2022
@kurkle
Copy link
Member

kurkle commented Jul 27, 2022

I'd like to explore the possibility to make the line element optional in the line controller. I'd think it would have smaller footprint overall, but am not sure.

@thabarbados thabarbados dismissed stale reviews from etimberg and LeeLenaleee via 181e5d3 July 29, 2022 09:05
@kurkle
Copy link
Member

kurkle commented Jul 29, 2022

Needs a rebase still

@thabarbados thabarbados force-pushed the remove-line-element-from-scatter branch from 181e5d3 to 1215635 Compare July 29, 2022 09:53
@thabarbados
Copy link
Contributor Author

Needs a rebase still

rebased

kurkle
kurkle previously approved these changes Jul 29, 2022
etimberg
etimberg previously approved these changes Jul 29, 2022
@etimberg
Copy link
Member

@thabarbados it looks like the tests are consistently failing

LeeLenaleee
LeeLenaleee previously approved these changes Jul 31, 2022
@thabarbados thabarbados dismissed stale reviews from LeeLenaleee, etimberg, and kurkle via 9418f97 August 1, 2022 09:34
@thabarbados
Copy link
Contributor Author

@thabarbados it looks like the tests are consistently failing

fixed

@kurkle kurkle merged commit 03e9194 into chartjs:master Aug 1, 2022
@typpo
Copy link
Contributor

typpo commented Nov 19, 2022

Noticing that v3.9.0 altered scatter plots that set line properties such as fill: true, but did not set showLine: true.

I realize support without showLine was probably never intended, but I'm leaving a note here for posterity in case anyone else encounters this!

Simple test case:

{
  type: 'scatter',
  data: {
    datasets: [{
      data: [{x:1,y:1}, {x:2,y:2}],
      backgroundColor: '#de000055',
      fill: true,
    }]
  }
}

Before (3.8.2)
image

After (3.9.0)
image

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

Successfully merging this pull request may close these issues.

6 participants