-
Notifications
You must be signed in to change notification settings - Fork 273
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/kx48zkosu |
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
==========================================
- Coverage 22.25% 22.17% -0.08%
==========================================
Files 265 265
Lines 6521 6549 +28
Branches 591 597 +6
==========================================
+ Hits 1451 1452 +1
- Misses 5033 5060 +27
Partials 37 37
Continue to review full report at Codecov.
|
@@ -18,7 +18,7 @@ const EMPTY = {}; | |||
|
|||
export type PromiseOrValue<T> = Promise<T> | T; | |||
export type PromiseOrValueLoader<T> = () => PromiseOrValue<T>; | |||
export type ChartType = ComponentType<any> | FunctionComponent<any>; | |||
export type ChartType = ComponentType<any>; |
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.
ComponentType
already includes FunctionComponent
const { colorScheme, groupby, metrics } = formData; | ||
const { colorScheme } = formData; | ||
const groupby = formData.groupby as string[]; | ||
const metrics = formData.metrics as string[]; |
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.
Maybe expand the ChartProps
like BigNumber for consistency?
I'm wondering whether we can add these generic formData
properties to the global formData
, too. They would go hand-in-hand with the shared controls.
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.
Great idea.
Btw, why does |
Good question. I do not remember about this either. cc @williaster who wrote the connection package |
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.
Late to the party, but went through this for the record, and LGTM.
🏠 Internal
Fix another hundred or so of lint warnings. Down from 199 warnings to < 10.