Skip to content

/ Chart.js

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

Jump to bottom

Use null for skipped values instead of NaN #8510

Merged
merged 10 commits into from Feb 24, 2021
Merged

Use null for skipped values instead of NaN #8510

merged 10 commits into from Feb 24, 2021
+37 −35

Conversation

@benmccann
Copy link
Collaborator

@benmccann benmccann commented Feb 24, 2021 •
edited

Allow charts to use data directly from server with skipped values when parsing: false. We can't send NaN from the server since it's not a valid JSON value
@benmccann benmccann force-pushed the benmccann:skip-null branch 3 times, most recently from ba634ff to 21885fb Feb 24, 2021
@benmccann benmccann marked this pull request as ready for review Feb 24, 2021
@benmccann
Use null for skipped values instead of NaN
Loading status checks…
b53b543
@benmccann benmccann force-pushed the benmccann:skip-null branch from 21885fb to b53b543 Feb 24, 2021
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 24, 2021

IMO it would make sense to skip all of the null, NaN and undefined. At least I'd keep skipping the NaN for less headache in migration from v2.

It would probably make sense to have a helper to determine if a parsed value should be skipped (public one for extensions to utilize too).
@kurkle kurkle added the type: enhancement label Feb 24, 2021
@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Feb 24, 2021

We do still skip NaN when parsing: true. This PR only affects parsing: false which is a new feature in v3, so hopefully shouldn't cause any migration difficulties (especially since it's so hard to get NaN data from the server in the first place).
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 24, 2021

We do still skip NaN when parsing: true. This PR only affects parsing: false which is a new feature in v3, so hopefully shouldn't cause any migration difficulties (especially since it's so hard to get NaN data from the server in the first place).

Ok, that is not obvious from the docs, currently I'd think only null value will be skipped. Looks like the parsing option is in the data structures page. Maybe we should link to that where we mention the null value (and maybe write some additional information on the data structures page, or even create a dedicated page for parsing).
@benmccann
Document skipped values when parsing is false
Loading status checks…
0d9cddc
@kurkle
kurkle reviewed Feb 24, 2021
View changes
Copy link
Collaborator

@kurkle kurkle left a comment

So in general, parsed should have null when the point is to be skipped.

Some of the getPixel* helpers are returning NaN for skipped (are all?). So the controllers that check for the x/y returned by these functions should still keep checking isNaN, while those that operate directly on parsed need to check for null.

We are probably missing some tests, because this already goes through. (Thinking bubble with null/NaN/undefined data points at least)
src/core/core.datasetController.js Outdated Show resolved Hide resolved
src/core/core.datasetController.js Outdated Show resolved Hide resolved
src/core/core.datasetController.js Outdated Show resolved Hide resolved
src/core/core.datasetController.js Outdated Show resolved Hide resolved
src/scales/scale.linear.js Show resolved Hide resolved
docs/docs/developers/axes.md Show resolved Hide resolved
benmccann and others added 2 commits Feb 24, 2021
@benmccann @kurkle
Update src/core/core.datasetController.js
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
fed3333 
Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com>
@benmccann @kurkle
Update src/core/core.datasetController.js
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
09b4d35 
Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com>
@kurkle
kurkle reviewed Feb 24, 2021
View changes
src/controllers/controller.bubble.js Outdated Show resolved Hide resolved
@benmccann
fix lint issue
Loading status checks…
c00d82c
@kurkle
kurkle reviewed Feb 24, 2021
View changes
src/controllers/controller.doughnut.js Outdated Show resolved Hide resolved
benmccann added 4 commits Feb 24, 2021
@benmccann
use isFinite
Loading status checks…
2f1c327
@benmccann
revert change checking for pixel values
Loading status checks…
78505bb
@benmccann
ternary readability
Loading status checks…
cda00ac
@benmccann
revert accidental paren movement
Loading status checks…
d7bbc57
@kurkle
kurkle reviewed Feb 24, 2021
View changes
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Feb 24, 2021

Filename Size Change
dist/chart.esm.js 66 kB +58 B (0%)
dist/chart.js 83.6 kB +57 B (0%)
dist/chart.min.js 59.4 kB +40 B (0%)
dist/chunks/helpers.segment.js 18.7 kB -1 B (0%)
dist/helpers.esm.js 1.06 kB -2 B (0%)
@etimberg
etimberg reviewed Feb 24, 2021
View changes
Copy link
Member

@etimberg etimberg left a comment

Looks good, but I think we need a test
@benmccann
test with parsing: false
Loading status checks…
53ee322
@etimberg
etimberg approved these changes Feb 24, 2021
View changes
@kurkle
kurkle approved these changes Feb 24, 2021
View changes
@etimberg etimberg merged commit 7c75310 into chartjs:master Feb 24, 2021
10 checks passed
10 checks passed
build (ubuntu-latest)
Details
build
Details
build (windows-latest)
Details
finish
Details
Coveralls - ubuntu-latest-chrome Coverage increased (+0.3%) to 94.921%
Details
Coveralls - ubuntu-latest-firefox Coverage increased (+0.3%) to 94.818%
Details
Coveralls - windows-latest-chrome Coverage increased (+0.3%) to 94.921%
Details
Coveralls - windows-latest-firefox Coverage increased (+0.2%) to 94.803%
Details
codeclimate All good!
Details
coverage/coveralls Coverage increased (+0.3%) to 94.921%
Details
@etimberg etimberg added this to the Version 3.0 milestone Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etimberg etimberg

@kurkle kurkle

Assignees
No one assigned
Labels
type: enhancement
Projects
None yet
Milestone
Version 3.0
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@benmccann @kurkle @etimberg