Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: re-type vx/scale with new functionalities #766
feat: re-type vx/scale with new functionalities #766
Changes from 3 commits
047140d
26277e6
7201124
cc311e1
a2e83f0
707e87b
81408f3
a7ab837
c52affc
48e2c4a
c5a718c
21c738a
bbebebf
b57bc94
783e56a
28a3783
56e0a47
f5fb093
7f45b41
41f4229
8d772a5
c948e0e
6ef370d
bad21bb
a0a24c2
deb50d6
aabe8e3
32cee04
dc8786f
653693d
90a5fee
eff7091
820c706
2c7152e
d9d21f2
e9eb41c
9e24d65
70a9a66
5c2f2e9
fe8d0b2
a13314a
27f6836
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Since these are mutable functions, is call order significant here? Does
applyInterpolate()
have to be called beforeapplyRound()
? If so, we might want to think of a way to enforce this. I don't expect it to a problem as it is lib code, but could be an easily introduced bug that could slip through a review.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.
Thank you for the comments. I think the order of operations is a valid point. I could set up a proxy function and only allow a single apply call with list of operations through this proxy to ensure order. Not sure if that is an overkill.
The ones that really have to be in order are
domain > nice > zero
.Not necessary. They should be mutually exclusive.
applyInterpolate()
is in factapplyColorInterpolate
. (This is a new functionality forvx
. I could also drop it. Previously I don't thinkvx
was acceptinginterpolate
.)Users can either do rounding which will set
interpolateRound
when a scale output is number.
or
when a scale output is color and they want to set color interpolation to HCL
It is still not super clean and the typing may allow something like this to be specified. Although both effect cannot happen at the same time.
Alternatives
interpolate
feature for now.{ interpolate }
or{ round }
. Can be complicated.{ round: true }
become{ interpolate: 'round' }
.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.
One idea instead of proxy (can copy & paste in browser console):
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.
I refactor with this idea. The individual scale files now look very clean.