Skip to content

DRY up color attributes #609

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 15 commits into from
Jun 7, 2016
Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Jun 6, 2016

This is a PR that is a prequel to #581 (covered with a PoC already) with the purpose of centralizing shared color attribute specifications. This is a non-functional change that consolidates attribute lines but should leave operations identical, beyond the odd case where the new version sometimes has more descriptive descriptions (the former code sometimes used marker even when the reference was included in marker.line). The new vertex/line color spec solution for #581 can thus use the new color spec attributes in bulk, without having to add them one by one.


'use strict';

module.exports = function makeColorScaleAttributes(context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good!

@monfera monfera force-pushed the 581a-dry-color-attributes branch from 925aa58 to eb4d42a Compare June 7, 2016 05:33
@monfera
Copy link
Contributor Author

monfera commented Jun 7, 2016

Outstanding reviewables:

  • RM: manual tests
  • EP: is it OK to extend on colorscale/attributes.js despite that having _deprecated props, or should the deprecated props be deleted, or the normal props defined in a separate file and the deprecated props would be looped in strictly where they were?
  • EP: surface/attributes.js has the structure of colorscale/attributes.js but with colorscale/color_attributes.js names - any advice for normalization, or should it be left as it is for now?
  • RM: delete the code comment above (on nesting colorspec attributes)

@monfera monfera force-pushed the 581a-dry-color-attributes branch 2 times, most recently from 7595849 to ab843f5 Compare June 7, 2016 08:18
@monfera monfera force-pushed the 581a-dry-color-attributes branch from ab843f5 to 99abf13 Compare June 7, 2016 09:03
@monfera monfera changed the title [WIP] 581a dry color attributes 581a dry color attributes Jun 7, 2016
@etpinard
Copy link
Contributor

etpinard commented Jun 7, 2016

is it OK to extend on colorscale/attributes.js despite that having _deprecated props, or should the deprecated props be deleted, or the normal props defined in a separate file and the deprecated props would be looped in strictly where they were?

I would prefer leaving _deprecated attribute separate. More clearly :

  • scl and reversescl should be only listed in colorscales/attributes.js
  • surface zauto, zmin and zmax should only be listed in surface/attributes.js

surface/attributes.js has the structure of colorscale/attributes.js but with colorscale/color_attributes.js names - any advice for normalization, or should it be left as it is for now?

Your call. I'm ok with leaving the surface variation as is or make colorscaleAttr take two arguments on representing the context and the other the color letter i.e. c or z.

@monfera
Copy link
Contributor Author

monfera commented Jun 7, 2016

@etpinard thanks! As a clarification: while scl and reversescl were present in colorscales/attributes.js, most of the actual traces have not included them, because prop inclusion used to be prop by prop, and usually, only the non-deprecated plots were included. From your answer I infer that you'd rather that we not introduce any deprecated stuff where it wasn't there before.

@etpinard
Copy link
Contributor

etpinard commented Jun 7, 2016

while scl and reversescl were present in colorscales/attributes.js, most of the actual traces have not included them,

Nice catch!

From your answer I infer that you'd rather that we not introduce any deprecated stuff where it wasn't there before.

Exactly. Let's get rid of those deprecated scl and reversescl attributes.

@monfera
Copy link
Contributor Author

monfera commented Jun 7, 2016

@etpinard can I remove them in general? I.e. even from colorscales/attributes.js and possible logic which may be specific to these two deprecated props?

@etpinard
Copy link
Contributor

etpinard commented Jun 7, 2016

can I remove them in general?

More clearly, remove this block, but keep this one.

@monfera
Copy link
Contributor Author

monfera commented Jun 7, 2016

OK, Limbo Stage 2 for these props 😼

' `cmin` and `cmax` if set.'
].join('')
},
colorscale: extendDeep({}, colorScaleAttributes.colorscale, {
Copy link
Contributor

Choose a reason for hiding this comment

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

extendFlat would suffice here, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes in general I might have been overly defensive with extendDeep. Should I convert all these extendDeep calls to extendFlat? It would work unless someone mutated the attribute object by accident.

@monfera monfera changed the title 581a dry color attributes DRY up color attributes Jun 7, 2016
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.

2 participants