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.

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

Abbreviation for yellow in colorscale names is 'Yl' not 'YI' [fixes #269] #295

Merged
merged 4 commits into from
Mar 1, 2016

Conversation

etpinard
Copy link
Contributor

- add backward compatible clean data step
- add cleanData tests
@etpinard etpinard changed the title Abbreviation for yellow in colorscale names is 'Yl' not 'YI [fixes #269] Abbreviation for yellow in colorscale names is 'Yl' not 'YI' [fixes #269] Feb 29, 2016
@@ -750,6 +751,17 @@ function cleanData(data, existingData) {
}
}

// fix typo in colorscale definition
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe put the check for YIGnBu/YIOrBu in a attribute coercion step instead of in logic code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want YIGnBu and YIOrBu to be considered valid colorscale names, we just want to clean up an old typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... unless we want to consider YIGnBu an alias for YlGnBu.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, just seems hacky to have it being handled in plot_api.js instead of in the colorscale component - if something uses colorscale but doesn't flow through this codepath, it won't get the same treatment, whereas I'd assume that there's a common entry for colorscale related things in colorscale/____.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cleanData is pretty up-front about its purpose.

@mdtusz
Copy link
Contributor

mdtusz commented Feb 29, 2016

Okey dokes. 💃

@alexcjohnson
Copy link
Collaborator

👍

- to reflect spaceless color strings in colorscale
  definitions.
etpinard added a commit that referenced this pull request Mar 1, 2016
Abbreviation for yellow in colorscale names is 'Yl' not 'YI' [fixes #269]
@etpinard etpinard merged commit 8dd98a0 into master Mar 1, 2016
@etpinard etpinard deleted the Yl-not-YI branch March 1, 2016 15:24
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