Skip to content
New issue

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

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

Already on GitHub? # to your account

Make Chart.layout(Service) importable #5113

Merged
merged 1 commit into from
Jan 7, 2018

Conversation

simonbrunel
Copy link
Member

Rename (and deprecate) Chart.layoutService to Chart.layout and make it importable.

Relates to #4478

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 7, 2018
@simonbrunel simonbrunel requested a review from etimberg January 7, 2018 17:06
benmccann
benmccann previously approved these changes Jan 7, 2018
Rename (and deprecate) `Chart.layoutService` to `Chart.layout` and make it importable.
@simonbrunel
Copy link
Member Author

Fixed a comment typo!

@simonbrunel simonbrunel merged commit ce27fe5 into chartjs:master Jan 7, 2018
@simonbrunel simonbrunel deleted the layout-module branch January 7, 2018 22:38
@@ -12,14 +12,14 @@ Chart.defaults = require('./core/core.defaults');
Chart.Element = require('./core/core.element');
Chart.elements = require('./elements/index');
Chart.Interaction = require('./core/core.interaction');
Chart.layout = require('./core/core.layout');
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I'm just wondering now, do we actually need a Chart.layout variable? shouldn't folks just call require('./core/core.layout') when they need the layout?

Copy link
Member Author

@simonbrunel simonbrunel Jan 7, 2018

Choose a reason for hiding this comment

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

The layout (singleton) can be accessed from external plugins/charts, in which case it's not possible to require './core/core.layout'.

Copy link
Member Author

Choose a reason for hiding this comment

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

@etimberg @benmccann Actually, I don't know why the layout is a singleton instead of an object attached to each chart and accessible via chartInstance.layout. Maybe that something we will like to change in v3.

For the alias, I would suggest another name to be consistent with other "Service" singletons: Chart.layouts (plural since it manages many layouts), as we have Chart.plugins and soon Chart.animations and Chart.scales.

What do you think?

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Rename (and deprecate) `Chart.layoutService` to `Chart.layout` and make it importable.
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants