Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix: 🐛 Fixed type definition for chart plugin #123

Merged
merged 3 commits into from
Mar 26, 2019
Merged

Conversation

conglei
Copy link
Contributor

@conglei conglei commented Mar 25, 2019

This PR is to fix the type definiton for transformProps and LoadData.

For TransformProps, the input type should be ChartProps instead of PlainObject.

For LoadData, before, the ChartType was ComponentType | FunctionComponent, which means the no props allowed for the component, which is not correct.

💔 Breaking Changes

🏆 Enhancements

Exported Metric definition.

📜 Documentation

🐛 Bug Fix

In most of the cases, There will be no PreTransformProps defined. Thus, the input for `TransformProp

🏠 Internal

Conglei Shi added 2 commits March 24, 2019 18:16
This PR is to fix the type definiton for transformProps and LoadData.
Change the output of preTransformProps to ChartProps
@conglei conglei requested a review from a team as a code owner March 25, 2019 03:08
@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #123 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #123   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          76     76           
  Lines         965    970    +5     
  Branches      232    232           
=====================================
+ Hits          965    970    +5
Impacted Files Coverage Δ
...ckages/superset-ui-chart/src/models/ChartPlugin.ts 100% <ø> (ø) ⬆️
...ges/superset-ui-color/src/CategoricalColorScale.ts 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b8efc2...7aca55d. Read the comment docs.

export type PreTransformProps = TransformFunction<ChartProps>;
export type TransformProps = TransformFunction;
export type PreTransformProps = TransformFunction<ChartProps, ChartProps>;
export type TransformProps = TransformFunction<ChartProps>;
Copy link
Contributor Author

@conglei conglei Mar 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do like

export type PreTransformProps = TransformFunction<ChartProps, ChartProps | PlainProps>;
export type TransformProps = TransformFunction<ChartProps | PlainProps>;`

However, in the chart plugin repo, if I define TransfromProps as

export default function transformProps(chartProps: ChartProps)

it will show the error: Type 'PlainProps | ChartProps' is not assignable to type 'ChartProps'.

Fixed the test according to the changes:
@conglei conglei added reviewable Ready for review #bug Something isn't working labels Mar 25, 2019
@kristw kristw changed the title fix: 🐛 Fixed typ definiton for chart plugin fix: 🐛 Fixed type definition for chart plugin Mar 25, 2019
@conglei conglei merged commit 6230a3b into master Mar 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the conglei_fix_types branch March 26, 2019 16:18
@xtinec
Copy link
Contributor

xtinec commented Mar 26, 2019

🎉 Thanks for fixing this. 💪 Noticed the same thing yesterday. Beat me to the fix. 🙂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#bug Something isn't working reviewable Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants