Skip to content

Split scatter module #184

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

Merged
merged 10 commits into from
Jan 15, 2016
Merged

Split scatter module #184

merged 10 commits into from
Jan 15, 2016

Conversation

etpinard
Copy link
Contributor

@etpinard
Copy link
Contributor Author

By doing so, we get rid of two circular dependencies:

  • Scatter <-> Drawing and
  • Scatter <-> ErrorBars

'use strict';

module.exports = {
PTS_LINESONLY: 20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

slightly abusive, but it's the only way to clear us of circular dependencies while keeping things 🌴

@etpinard
Copy link
Contributor Author

Another thing to note, with this PR, no src file requires the full Scatter module (i.e. no files has traces/scatter/index directly or Plotly.Scatter). So, 'scatter' does not need to be part of the 'core' module for things to work.

That is, we can remove these lines in src/plotly.js.

That said, should we make the pro users register the Scatter? I'd vote for yes. Its plot step is quite big.

@mdtusz
Copy link
Contributor

mdtusz commented Jan 15, 2016

+1 for making pro users register all/any traces.

@mdtusz
Copy link
Contributor

mdtusz commented Jan 15, 2016

I'd say that in Plotly.plot, we should just throw an error if no traces have been registered.

@mdtusz
Copy link
Contributor

mdtusz commented Jan 15, 2016

This looks good to go though 💃

@etpinard
Copy link
Contributor Author

@mdtusz

I'd say that in Plotly.plot, we should just throw an error if no traces have been registered.

agreed. 👍

etpinard added a commit that referenced this pull request Jan 15, 2016
@etpinard etpinard merged commit 2e59e7a into master Jan 15, 2016
@etpinard etpinard deleted the split-scatter branch January 15, 2016 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants