-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
|
||
'use strict'; | ||
|
||
module.exports = function makeColorScaleAttributes(context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
925aa58
to
eb4d42a
Compare
Outstanding reviewables:
|
7595849
to
ab843f5
Compare
ab843f5
to
99abf13
Compare
I would prefer leaving
Your call. I'm ok with leaving the surface variation as is or make |
@etpinard thanks! As a clarification: while |
Nice catch!
Exactly. Let's get rid of those deprecated |
@etpinard can I remove them in general? I.e. even from |
OK, Limbo Stage 2 for these props 😼 |
' `cmin` and `cmax` if set.' | ||
].join('') | ||
}, | ||
colorscale: extendDeep({}, colorScaleAttributes.colorscale, { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 inmarker.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.