Skip to content

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

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