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.

Sign up for GitHub

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

Allow switching platforms #6964

Merged
merged 8 commits into from
Jan 26, 2020
Merged

Allow switching platforms #6964

merged 8 commits into from
Jan 26, 2020

Conversation

davidswinegar
Copy link
Contributor

@davidswinegar davidswinegar commented Jan 15, 2020 •
edited
Loading

Move the Chart.platform to live as an instance on the chart instead, and add ways to override the current platform. This is necessary for adding tests that use the "basic" platform - which will be needed to test Chart.js in a web worker.

@davidswinegar
Allow switching platforms
f8945ea 
Move the Chart.platform to Chart.platform.current instead, and add
ways to see available platforms and set the current platform. This
is necessary for adding tests that use the "basic" platform.
@benmccann
Copy link
Contributor

You'll want to mention these changes in the migration guide: https://github.com/chartjs/Chart.js/blob/master/docs/getting-started/v3-migration.md

@davidswinegar
Copy link
Contributor Author

davidswinegar commented Jan 15, 2020 •
edited
Loading

@benmccann I've looked over the next steps and I think I might want to rework this a bit. What do you think about storing a platform on each chart instance? That way users can create charts using the dom platform and basic platform separately.
Some simple rules:

  • users can override the platform used, but by default Canvas elements will have the "dom" platform if window & document are both defined, and OffscreenCanvas elements will have the "basic" platform. We can detect window.OffscreenCanvas. Override is done by instance (overridePlatform: new BasicPlatform())
  • The other option we would to support on chart creating is disableCSSInjection, which is currently a platform.dom property.

@benmccann
Copy link
Contributor

@kurkle and @etimberg might have thoughts, but that seems reasonable to me

etimberg
etimberg reviewed Jan 15, 2020
View reviewed changes
Copy link
Member

@etimberg etimberg 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. I think allowing switching on chart create is a good idea. It will allow more flexibility

src/platforms/platform.js Outdated Show resolved Hide resolved
@davidswinegar
Copy link
Contributor Author

@benmccann @etimberg thanks for the feedback! I updated this with the switch to a per-chart platform.

kurkle
kurkle reviewed Jan 16, 2020
View reviewed changes
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Not much to say, looks good to me!

src/platforms/platform.dom.js Outdated Show resolved Hide resolved
benmccann
benmccann reviewed Jan 16, 2020
View reviewed changes
docs/getting-started/v3-migration.md Outdated Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
src/platforms/platform.dom.js Outdated Show resolved Hide resolved
src/platforms/platform.js Outdated Show resolved Hide resolved
@davidswinegar davidswinegar force-pushed the dwinegar-allow-switching-platforms branch from e3b4185 to c005cad Compare January 17, 2020 01:38
@benmccann
Copy link
Contributor

this looks pretty good to me. the build is failing, so you'll need to get the tests passing

@davidswinegar
Copy link
Contributor Author

Got them working! Thanks for the feedback and quick responses everyone.

benmccann
benmccann previously approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

Cool! I'm very curious to learn more about possible web worker support :-)

etimberg
etimberg previously approved these changes Jan 18, 2020
View reviewed changes
@davidswinegar davidswinegar dismissed stale reviews from etimberg and benmccann via 1c297ab January 21, 2020 22:38
benmccann
benmccann previously approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

thanks for the patience on the reviews

samples/advanced/content-security-policy.js Outdated
@@ -48,7 +47,7 @@ window.addEventListener('load', function() {
return (size / 24) * base;
}
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: trailing comma

etimberg
etimberg previously approved these changes Jan 21, 2020
View reviewed changes
@davidswinegar davidswinegar dismissed stale reviews from etimberg and benmccann via b2561d7 January 22, 2020 18:54
@davidswinegar
Copy link
Contributor Author

@benmccann fixed that last issue. Looks like there's some codeclimate issues but none of them include code I touched - I think they're just there because of the merge commit I included.

@etimberg etimberg merged commit 1ad5f36 into chartjs:master Jan 26, 2020
@etimberg etimberg added this to the Version 3.0 milestone Jan 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmccann benmccann benmccann approved these changes

@etimberg etimberg etimberg approved these changes

@kurkle kurkle kurkle approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@davidswinegar @benmccann @etimberg @kurkle