-
Notifications
You must be signed in to change notification settings - Fork 712
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
typescript(vx-legend): re-write package in TypeScript #551
Conversation
@@ -40,6 +40,8 @@ const thresholdScale = scaleThreshold({ | |||
range: ['#f2f0f7', '#dadaeb', '#bcbddc', '#9e9ac8', '#756bb1', '#54278f'], | |||
}); | |||
|
|||
global.s = thresholdScale; |
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.
testing, will remove
'labelDelimiter' | 'labelLower' | 'labelUpper' | ||
>): LabelFormatterFactory<Datum, Output, ScaleThreshold<Datum, Output>> { | ||
return ({ scale, labelFormat }) => (d, i) => { | ||
let [x0, x1] = scale.invertExtent(scale.range()[i]); |
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.
this is messy, the problem is that the previous implementation used scale.range()
and passed that as the domain
prop. that is no longer valid with types, and it's impossible to compute the right thresholds directly from the domain value d
. the current implementation assumes that domain.length === range.length
👎
the domain of a threshold
scale also is not guaranteed to be a number
so we have extra checks for that here.
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.
after going through the d3 docs more, if domain
has n
items then the range
of a valid scale should have n+1
, so I think this is safe. I updated the logic to check that the item corresponding to index i
is within the range, and added a comment. we can update in the future if need be.
export type ScaleThreshold<Input extends BaseInput, Output> = d3Scale.ScaleThreshold<Input, Output>; | ||
export type ScaleQuantile<Output> = d3Scale.ScaleQuantile<Output>; | ||
|
||
// @TODO BaseInput only needed for `ScaleThreshold` |
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'm also not sure how to get around this cleanly. only ScaleThreshold
has restrictions on Input
, but that vastly restricts uses of the others. @kristw do you have any advance TS
ideas?
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.
might be able to use conditional types here
type ScaleType<Input, Output> = Input extends BaseInput
? ScaleThreshold<Input, Output> | NonThresholdType<Input, Output>
: NonThresholdType<Input, Output>;
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 spent a lot of time on this and couldn't get the conditional typing to work 😱 I am TS-ignoring it for now and will sync with @kristw and others when they're back from holiday.
...restProps | ||
}: LegendQuantileProps<Output>) { | ||
// transform range into input values because it may contain more elements | ||
const domain = inputDomain || scale.range().map(output => scale.invertExtent(output)[0]); |
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.
this similarly just passed scale.range()
as domain
previously, this works correctly though. the note about the range
I don't think is necessarily always true.
1c11bc2
to
aef49d3
Compare
woot! finally 😅 |
🚀 Enhancements
This PR builds off #488 which introduces
Typescript
build config, and re-writes the@vx/legend
package in TypeScript.This one was a bit gnarly vs some. Closes #546.
💥 Breaking Changes
React.Fragments
, which requires bumping thepeerDep
for react to^16.3.0-0
index
is unchanged):src/legends/*
=>src/*
Tests
/legend
demo.d.ts
filesScreen shots
Prod
Local
@hshoff @kristw