feat(*): added scale radial #958
Conversation
|
Had to update the version of |
sarathps93
force-pushed
the
feature/scale-radial
branch
4 times, most recently
from
fde0109
to
7de8acc
Compare
| @@ -11,6 +11,6 @@ export default function applyUnknown< | |||
| config: ScaleConfigWithoutType<Output, DiscreteInput, ThresholdInput>, | |||
| ) { | |||
| if ('unknown' in scale && 'unknown' in config && typeof config.unknown !== 'undefined') { | |||
| scale.unknown(config.unknown); | |||
| (scale.unknown as any)(config.unknown); | |||
There was a problem hiding this comment.
The latest type definitions for d3-scales are causing issues at here and that's why I have including the type casting in here
There was a problem hiding this comment.
Would you mind sharing the specific issue?
Just want to help think if there is a workaround that is more type-safe than casting to any.
There was a problem hiding this comment.
/**
* Returns the current unknown value, which defaults to undefined.
*/
unknown(): UnknownReturnType<Unknown, undefined>;
/**
* Sets the output value of the scale for undefined (or NaN) input values and returns this scale.
*
* @param value The output value of the scale for undefined (or NaN) input values.
*/
unknown<NewUnknown>(value: NewUnknown): ScaleLinear<Range, Output, NewUnknown>;Above is the latest type definition from @types/d3-scale. Somehow the scale.unknown is pointing to the function which accepts no arguments.
There was a problem hiding this comment.
@kristw Please let me if any other workaround can be done to avoid this.
There was a problem hiding this comment.
It looks like TS isn't correctly matching the second overload. is there any way to import NewUnknown and coerce to that?
scale.unknown(config.unknown as NewUnknown)There was a problem hiding this comment.
@williaster I have tried that option as well. They are not exposing the NewUnknown for us to import
There was a problem hiding this comment.
@kristw any other thoughts? else we can probably merge this as is
Pull Request Test Coverage Report for Build 401423712
💛 - Coveralls |
sarathps93
force-pushed
the
feature/scale-radial
branch
from
7de8acc
to
65be419
Compare
There was a problem hiding this comment.
This looks great to me with one minor comment on the RadialScaleConfig.
Also cc-ing @kristw in case he notices anything else since he's very familiar with the scale package.
| 'radial', | ||
| ContinuousDomain, | ||
| Output[], | ||
| 'clamp' | 'nice' | 'round' | 'zero' | 'unknown' |
There was a problem hiding this comment.
should zero be omitted here since it's not applied in scales/radial.ts?
There was a problem hiding this comment.
Yeah makes sense. Have done the changes
sarathps93
force-pushed
the
feature/scale-radial
branch
from
65be419
to
3ba3008
Compare
There was a problem hiding this comment.
Spoke with @kristw offline, let's merge this! Thanks again for the contribution 🙏
Pull Request Test Coverage Report for Build 401423712Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
🚀 Enhancements
Radial Scalewhich will help in creating charts likecircular bar plot. Adding it as a part of [feature request]: radial bar chart #832