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

Add raw data to context and rename dataPoint to parsed #8318

Merged
merged 3 commits into from Feb 5, 2021
Merged

Add raw data to context and rename dataPoint to parsed #8318

merged 3 commits into from Feb 5, 2021
+31 −24

Conversation

@benmccann
Copy link
Collaborator

@benmccann benmccann commented Jan 16, 2021

Simplifies writing callbacks as a user
@benmccann
Make the raw data point available in scriptable context
Loading status checks…
6de0bdf
@benmccann benmccann mentioned this pull request Jan 16, 2021
Merged
@etimberg
etimberg reviewed Jan 16, 2021
View changes
Copy link
Member

@etimberg etimberg left a comment

No issues with the implementation, but can we call this raw as it won't necessarily be a point object?
@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Jan 16, 2021

raw sounds fine to me, but would we also want to call dataPoint something else then?
@etimberg
Copy link
Member

@etimberg etimberg commented Jan 16, 2021

I think dataPoint is always a point-like object { x: , y: } since we parse to that
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Jan 16, 2021 •
edited

how about parsed and raw? should be quite obvious :)

Add: I had to check couple of times if the dataPoint was the raw one or the parsed one already. So that is not obvious.
@etimberg
Copy link
Member

@etimberg etimberg commented Jan 16, 2021

That sounds good to me
@etimberg
etimberg reviewed Jan 16, 2021
View changes
@benmccann
Rename variables
Loading status checks…
1d499a0
@benmccann benmccann force-pushed the benmccann:rawPoint branch from d8d6b7b to 1d499a0 Jan 16, 2021
@benmccann benmccann changed the title Make the raw data point available in scriptable context Add raw data to scriptable context and rename dataPoint to parsed Jan 16, 2021
@benmccann benmccann changed the title Add raw data to scriptable context and rename dataPoint to parsed Add raw data to context and rename dataPoint to parsed Jan 16, 2021
@etimberg
etimberg reviewed Jan 16, 2021
View changes
@kurkle
kurkle reviewed Jan 18, 2021
View changes
Copy link
Collaborator

@kurkle kurkle left a comment

Looks ok.

Searching dataPoint from the source returns hits from 16 files, so you probably missed some?
@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Jan 18, 2021

I don't think I missed any. There are still several variables left with the name dataPoints so maybe that's what you're seeing
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Jan 18, 2021

For example:

Chart.js/samples/animations/delay.html

Line 79 in 4f74a92

if (context.dataPoint && !context.delayed) {

@benmccann
Update samples
Loading status checks…
bf15077
@benmccann
Copy link
Collaborator Author

@benmccann benmccann commented Jan 24, 2021

Ah, I forgot about the samples. Thanks for catching that! Updated
@etimberg
etimberg approved these changes Jan 27, 2021
View changes
@kurkle
Copy link
Collaborator

@kurkle kurkle commented Jan 28, 2021

The last thing I can spot still missing is the types:

Chart.js/types/index.esm.d.ts

Lines 40 to 47 in 8cca4fe

export interface ScriptableContext {
chart: Chart;
dataPoint: any;
dataIndex: number;
dataset: ChartDataset;
datasetIndex: number;
active: boolean;
}

@kurkle
kurkle approved these changes Feb 5, 2021
View changes
Copy link
Collaborator

@kurkle kurkle left a comment

Looks like @benmccann is quite busy, we can update the types separately.
@etimberg etimberg added this to the Version 3.0 milestone Feb 5, 2021
@etimberg
Copy link
Member

@etimberg etimberg commented Feb 5, 2021

Sounds good, we can merge then update the types
@etimberg etimberg merged commit eb7ce4e into chartjs:master Feb 5, 2021
9 of 10 checks passed
9 of 10 checks passed
build (ubuntu-latest)
Details
build
Details
build (windows-latest)
Details
finish
Details
codeclimate 2 issues to fix
Details
Coveralls - ubuntu-latest-chrome First build on rawPoint at 95.845%
Details
Coveralls - ubuntu-latest-firefox First build on rawPoint at 95.723%
Details
Coveralls - windows-latest-chrome First build on rawPoint at 95.845%
Details
Coveralls - windows-latest-firefox First build on rawPoint at 95.723%
Details
coverage/coveralls First build on rawPoint at 95.845%
Details
@etimberg etimberg mentioned this pull request Feb 5, 2021
Closed
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 @etimberg @kurkle