Add SuperChart #68

kristw · 2018-12-17T19:27:26Z

🏆 Enhancements

Migrate SuperChart from incubator-superset, convert to TypeScript and add 100% unit tests.
Add default values for ChartProps

🏠 Internal

Change to export default for some modules.

codecov · 2018-12-17T19:31:21Z

Codecov Report

Merging #68 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #68   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          66     67    +1     
  Lines         669    734   +65     
  Branches      106    118   +12     
=====================================
+ Hits          669    734   +65

Impacted Files	Coverage Δ
...ages/superset-ui-chart/src/models/ChartMetadata.ts	`100% <ø> (ø)`	⬆️
...ackages/superset-ui-chart/src/models/ChartProps.ts	`100% <100%> (ø)`	⬆️
...ckages/superset-ui-chart/src/models/ChartPlugin.ts	`100% <100%> (ø)`	⬆️
...es/superset-ui-chart/src/components/SuperChart.tsx	`100% <100%> (ø)`

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 556a508...e9d6e10. Read the comment docs.

williaster

lgtm, had a couple misc questions about TS.

@xtinec I think a second eye on the TS could be good still if you have time!

williaster · 2018-12-18T05:02:31Z

packages/superset-ui-chart/src/components/SuperChart.tsx

+};
+/* eslint-enable sort-keys */
+
+type TransformFunction = (x: any) => any;


do you think there's any way to make this a generic? that would allow more informative typings but not sure if there's really anything meaningful we know about the arguments. they're minimally objects right? and we know pre accepts ChartProps?

Will update.

williaster · 2018-12-18T05:03:57Z

packages/superset-ui-chart/src/components/SuperChart.tsx

+  overrideTransformProps?: TransformFunction;
+  postTransformProps?: TransformFunction;
+  onRenderSuccess?: HandlerFunction;
+  onRenderFailure?: HandlerFunction;


do errors have a known / consistent signature? I guess if every chart handles them differently, probably not 😢

This one is currently hooked up for logging. And may update redux state.

williaster · 2018-12-18T05:16:25Z

packages/superset-ui-chart/src/components/SuperChart.tsx

+  ) => LoadableRenderer<RenderProps, LoadedModules> | (() => null);
+
+  renderChart(loaded: LoadedModules, props: RenderProps) {
+    const Chart = getModule<typeof React.Component>(loaded.Chart);


does this require the typeof prefix? when we use it elsewhere (e.g., LoadedModules or reactify) we don't need this 🤔 wonder if it has to do with the import * as React?

It does. I had <React.Component> at first and it fails.

There was a StackOverflow thread explaining it. <React.Component> will mean Chart is an instanceof React.Component but it is not what we want. Chart is a constructor that creates React.Component, not the React.Component instance, which is what <typeof React.Component> is.

williaster · 2018-12-18T05:21:00Z

packages/superset-ui-chart/src/components/SuperChart.tsx

+      id?: string;
+      className?: string;
+    } = {};
+    if (id) {


does this complain if you set it as this?

const containerProps: { id?: string; className?: string; } = { id, className };

This might still create <div id="" className=""> eventhough id and className are undefined. The if condition ensure it will be just <div>, which is slightly cleaner.

packages/superset-ui-chart/src/models/ChartPlugin.ts

packages/superset-ui-chart/test/components/SuperChart.test.tsx

xtinec · 2018-12-18T23:48:14Z

packages/superset-ui-chart/test/components/SuperChart.test.jsx

+describe('SuperChart', () => {
+  it('does not render if chartType is not set', done => {
+    const wrapper = shallow(<SuperChart />);
+    setTimeout(() => {


hmm, it seems we only need setTimeout when testing the slow chart, right?

xtinec · 2018-12-18T23:49:58Z

packages/superset-ui-chart/src/components/SuperChart.tsx

+
+const EMPTY = () => null;
+
+/* eslint-disable sort-keys */


Might be worth-while disabling sort-keys in our build-config.

yeah I have mixed feelings on that rule, it's a pain sometimes esp when you want to logically group things like this. I'll make the change in build-config.

xtinec · 2018-12-19T01:08:16Z

packages/superset-ui-chart/test/components/SuperChart.test.tsx

@@ -62,28 +62,28 @@ describe('SuperChart', () => {
      setTimeout(() => {
        expect(wrapper.render().find('div.test-component')).toHaveLength(1);
        done();
-      }, 5);
+      }, 0);


We could remove setTimeout altogether on all except for slow charts, perhaps?

expect(...); done();

tried and it fails tests if I remove setTimeout. This might be something to do with react-loadable

I've found that sometimes setTimeout(fn, 0) is needed to get to the next tick for some lifecycle or mount-based functionality.

Interesting that the "requeueing" of fn is necessary sometimes. We can leave this as-is then.

xtinec · 2018-12-19T01:37:47Z

packages/superset-ui-chart/src/components/SuperChart.tsx

+  chartProps?: ChartProps | null;
+  chartType: string;
+  preTransformProps?: TransformFunction<ChartProps>;
+  overrideTransformProps?: TransformFunction;


Should both overrideTransformProps and postTransformProps take ChartProps as type arguments?

If preTransformProps transforms ChartProps into something else, that will be input to overrideTransformProps and/or postTransformProps

That's true. I suppose we could create a generic over the output props and use that as the input for overrideTransformProps and postTransformProps.

I'm cool leaving this as-is for now (perhaps adding a TODO to revisit?). Think we could potentially improve the types here once we have more chart plugins added.

* fix: fallback to default margin when margin is partially set * feat: can disable axis title * feat: adjust margin according to axis title visibility * feat: include margin in formData * feat: add buildQuery * fix: address kyle comments * fix: remove string false * fix: line chart temporal scale * fix: format

kristw added 4 commits December 14, 2018 14:12

Add superchart and convert to ts

2bdc388

fix compilation error

0bbbc8e

fix build issue

6a84644

unit test working

8e397e7

kristw requested a review from a team as a code owner December 17, 2018 19:27

kristw added the WIP Work in progress label Dec 17, 2018

kristw added the #enhancement New feature or request label Dec 17, 2018

kristw added 5 commits December 17, 2018 13:49

Add default width and height to ChartProps

31da70f

export SuperChart

f6497e3

Add null check and update unit tests

38a4e02

update unit tests

e6dcb28

complete test coverage

c3e12d7

kristw added reviewable Ready for review and removed WIP Work in progress labels Dec 18, 2018

williaster reviewed Dec 18, 2018

View reviewed changes

Add generic

f0a89d0

kristw requested a review from xtinec December 18, 2018 21:18

xtinec reviewed Dec 18, 2018

View reviewed changes

reduce timeout

e9d6e10

xtinec reviewed Dec 19, 2018

View reviewed changes

xtinec approved these changes Dec 19, 2018

View reviewed changes

xtinec merged commit 9c48747 into master Dec 20, 2018

delete-merged-branch bot deleted the kristw--super-chart branch December 20, 2018 20:32

kristw added this to the v0.8.0 milestone Feb 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SuperChart #68

Add SuperChart #68

kristw commented Dec 17, 2018 •

edited

Loading

codecov bot commented Dec 17, 2018 •

edited

Loading

williaster left a comment

williaster Dec 18, 2018

kristw Dec 18, 2018

williaster Dec 18, 2018

kristw Dec 18, 2018 •

edited

Loading

williaster Dec 18, 2018

kristw Dec 18, 2018

williaster Dec 18, 2018

kristw Dec 18, 2018

xtinec Dec 18, 2018

xtinec Dec 18, 2018

williaster Dec 19, 2018

xtinec Dec 19, 2018

xtinec Dec 19, 2018

kristw Dec 19, 2018

williaster Dec 19, 2018

xtinec Dec 19, 2018

xtinec Dec 19, 2018

kristw Dec 19, 2018

xtinec Dec 19, 2018

Add SuperChart #68

Add SuperChart #68

Conversation

kristw commented Dec 17, 2018 • edited Loading

codecov bot commented Dec 17, 2018 • edited Loading

Codecov Report

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw commented Dec 17, 2018 •

edited

Loading

codecov bot commented Dec 17, 2018 •

edited

Loading

kristw Dec 18, 2018 •

edited

Loading