[SIP-6] Add reactify function and convert world map to new directory structure. #5893

kristw · 2018-09-14T01:15:14Z

This PR add utility functions reactify and createAdaptor which are necessary for directory structure described in SIP-6 (#5692 )

Also demonstrate usage with WorldMap

@conglei @williaster

williaster

Looks good overall, I had a couple comments/questions.

williaster · 2018-09-17T22:49:47Z

superset/assets/src/utils/reactify.jsx

+  }
+  if (renderFn.defaultProps) {
+    ReactifiedComponent.defaultProps = renderFn.defaultProps;
+  }


could we also check for + add displayName?

if (renderFn.displayName) { ReactifiedComponent.displayName = renderFn.displayName; }

williaster · 2018-09-17T22:54:59Z

superset/assets/src/visualizations/WorldMap/ReactWorldMap.js

+import reactify from '../../utils/reactify';
+import WorldMap from './WorldMap';
+
+export default reactify(WorldMap);


what do you think about just using WorldMap.jsx instead to denote react components instead of including React in the name?

My main thought is that this might make naming more consistent with visualizations that are React already (so wouldn't specify ReactMyVisComponent.jsx and instead would probably just be MyVisComponent.jsx), but would then require specifying a .jsx in the adaptor file (I'm personally okay with that for those that require it).

I am afraid I will make mistake when importing. If I specify ./WorldMap, which one will webpack resolve to first?

that's fair, I guess in a world where most visualizations are not react and will have react adapters, this is okay 👍

What do you think about them being ReactWorldMap.jsx vs *.js? I guess really there's no jsx in the file, so .js is probably the right call technically.

Oh good point. I can definitely rename it to .js

williaster · 2018-09-17T22:58:29Z

superset/assets/src/visualizations/models/BasicChartInput.js

+
+export default class BasicChartInput {
+  constructor(slice, payload, setControlValue) {
+    this.width = slice.width();


does this model support dynamic sizing easily? I think it'd be really nice to move away from width and height being functions on the slice object because it forces child to call them, or some component somewhere to add window resize listeners to then force the child to re-render (which differs depending on dashboard vs explorer, etc.), instead of auto-updating and cascading as props would.

With the new adaptor model I think we're pretty well poised to leverage the resize observer from vx's ParentSize component:

// createAdaptor.jsx ReactDOM.render( <ParentSize> {({ width, height }) => ( <Component width={width} height={height} {...transformProps(basicChartInput)} />, )} </ParentSize>, document.querySelector(slice.selector), );

That looks interesting. I will give this a try.

Doesn't work with the explore page yet due to the parent <div> from bootstrap panel does not know its height so when ParentSize tries to get height 100%, it ends up getting the entire screen height. I don't feel like changing the explore page div style in this PR, so I will pass the width and height from slice as separated argument from basicChartInput instead and remove this once I work on the Chart.jsx.

thanks for exploring (😉) it, sg 👍

conglei · 2018-09-17T22:49:09Z

superset/assets/src/utils/convertKeysToCamelCase.js

@@ -0,0 +1,11 @@
+import { mapKeys, camelCase, isPlainObject } from 'lodash/fp';
+
+export default function convertKeysToCamelCase(object) {


conglei · 2018-09-17T22:58:03Z

superset/assets/src/visualizations/WorldMap/adaptor.jsx

@@ -0,0 +1,5 @@
+import createAdaptor from '../../utils/createAdaptor';


I'm thinking how to make each visualization as self contained (independent with superset code) as possible. But it seems now it is hard to do so, otherwise there will be duplicated codes like this for each visualizations type.

Remember this file is temporary and will be removed when we move to entirely plugin system.

I think including it in @superset-ui/charts as a util function for the time being is okay.

conglei · 2018-09-17T22:59:11Z

superset/assets/src/visualizations/WorldMap/transformProps.js

@@ -0,0 +1,11 @@
+export default function transformProps(basicChartInput) {


Here we could do some propType check on formData.

that's an interesting idea. I think that doing that in another PR could be okay tho, it would probably be a decent amount of work to enumerate all of the options there. it's basically defining the formData interface / would be a precursor to the type script interface.

It could be part of the BasicChartInput or extended class.

For example, class PieChartInput extends BasicChartInput then in the constructor you parse formData with new PieChartFormData(formData).

Maybe we should use TypeScript after all lol.

conglei · 2018-09-17T23:02:51Z

superset/assets/src/visualizations/index.js

@@ -108,7 +108,7 @@ const vizMap = {
  [VIZ_TYPES.word_cloud]: () =>
    loadVis(import(/* webpackChunkName: "word_cloud" */ './wordcloud/WordCloud.js')),
  [VIZ_TYPES.world_map]: () =>
-    loadVis(import(/* webpackChunkName: "world_map" */ './world_map.js')),
+    loadVis(import(/* webpackChunkName: "world_map" */ './WorldMap/adaptor.jsx')),


Can we add index.js for each file and export adaptor there as default. Potentially we also want to just import the chart itself in the plugin system world.

index.js file per vis folder could be a good idea +1

This visualizations/index.js file and adaptor.jsx will be removed soon so I prefer not to create index.js to be another file waiting for deletion.

codecov-io · 2018-09-18T21:02:41Z

Codecov Report

Merging #5893 into master will decrease coverage by 15.6%.
The diff coverage is 11.29%.

@@             Coverage Diff             @@
##           master    #5893       +/-   ##
===========================================
- Coverage   63.66%   48.06%   -15.61%     
===========================================
  Files         386      393        +7     
  Lines       23538    23687      +149     
  Branches     2628     2669       +41     
===========================================
- Hits        14986    11385     -3601     
- Misses       8539    12301     +3762     
+ Partials       13        1       -12

Impacted Files	Coverage Δ
superset/assets/src/utils/createAdaptor.jsx	`0% <0%> (ø)`
superset/assets/src/utils/reactify.jsx	`0% <0%> (ø)`
...ssets/src/visualizations/WorldMap/ReactWorldMap.js	`0% <0%> (ø)`
...set/assets/src/visualizations/WorldMap/adaptor.jsx	`0% <0%> (ø)`
...set/assets/src/visualizations/WorldMap/WorldMap.js	`0% <0%> (ø)`
...sets/src/visualizations/WorldMap/transformProps.js	`0% <0%> (ø)`
...ssets/src/visualizations/models/BasicChartInput.js	`0% <0%> (ø)`
...uperset/assets/src/utils/convertKeysToCamelCase.js	`100% <100%> (ø)`
...perset/assets/src/visualizations/nvd3/PropTypes.js	`0% <0%> (-100%)`	⬇️
.../dashboard/util/charts/getEffectiveExtraFilters.js	`5.55% <0%> (-94.45%)`	⬇️
... and 206 more

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 19a3319...86ee56c. Read the comment docs.

codecov-io · 2018-09-18T21:02:47Z

Codecov Report

Merging #5893 into master will decrease coverage by 15.6%.
The diff coverage is 11.29%.

@@             Coverage Diff             @@
##           master    #5893       +/-   ##
===========================================
- Coverage   63.66%   48.06%   -15.61%     
===========================================
  Files         386      393        +7     
  Lines       23538    23687      +149     
  Branches     2628     2669       +41     
===========================================
- Hits        14986    11385     -3601     
- Misses       8539    12301     +3762     
+ Partials       13        1       -12

Impacted Files	Coverage Δ
superset/assets/src/utils/createAdaptor.jsx	`0% <0%> (ø)`
superset/assets/src/utils/reactify.jsx	`0% <0%> (ø)`
...ssets/src/visualizations/WorldMap/ReactWorldMap.js	`0% <0%> (ø)`
...set/assets/src/visualizations/WorldMap/adaptor.jsx	`0% <0%> (ø)`
...set/assets/src/visualizations/WorldMap/WorldMap.js	`0% <0%> (ø)`
...sets/src/visualizations/WorldMap/transformProps.js	`0% <0%> (ø)`
...ssets/src/visualizations/models/BasicChartInput.js	`0% <0%> (ø)`
...uperset/assets/src/utils/convertKeysToCamelCase.js	`100% <100%> (ø)`
...perset/assets/src/visualizations/nvd3/PropTypes.js	`0% <0%> (-100%)`	⬇️
.../dashboard/util/charts/getEffectiveExtraFilters.js	`5.55% <0%> (-94.45%)`	⬇️
... and 206 more

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 19a3319...86ee56c. Read the comment docs.

codecov-io · 2018-09-18T21:02:51Z

Codecov Report

Merging #5893 into master will decrease coverage by 15.6%.
The diff coverage is 11.29%.

@@             Coverage Diff             @@
##           master    #5893       +/-   ##
===========================================
- Coverage   63.66%   48.06%   -15.61%     
===========================================
  Files         386      393        +7     
  Lines       23538    23687      +149     
  Branches     2628     2669       +41     
===========================================
- Hits        14986    11385     -3601     
- Misses       8539    12301     +3762     
+ Partials       13        1       -12

Impacted Files	Coverage Δ
superset/assets/src/utils/createAdaptor.jsx	`0% <0%> (ø)`
superset/assets/src/utils/reactify.jsx	`0% <0%> (ø)`
...ssets/src/visualizations/WorldMap/ReactWorldMap.js	`0% <0%> (ø)`
...set/assets/src/visualizations/WorldMap/adaptor.jsx	`0% <0%> (ø)`
...set/assets/src/visualizations/WorldMap/WorldMap.js	`0% <0%> (ø)`
...sets/src/visualizations/WorldMap/transformProps.js	`0% <0%> (ø)`
...ssets/src/visualizations/models/BasicChartInput.js	`0% <0%> (ø)`
...uperset/assets/src/utils/convertKeysToCamelCase.js	`100% <100%> (ø)`
...perset/assets/src/visualizations/nvd3/PropTypes.js	`0% <0%> (-100%)`	⬇️
.../dashboard/util/charts/getEffectiveExtraFilters.js	`5.55% <0%> (-94.45%)`	⬇️
... and 206 more

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 19a3319...86ee56c. Read the comment docs.

kristw · 2018-09-18T21:55:59Z

Comments addressed.

williaster

LGTM

mistercrunch · 2018-09-20T15:24:20Z

@kristw @williaster I'm not quite sure why, but since this PR it looks like we've been running only a handful of javascript tests.

codecov-io is often out of whack, but it looks like it was in tune here detecting this.

kristw · 2018-09-20T15:45:18Z

I forgot to remove .only from one of the tests. I have fix for that in the deprecate getcolorfromscheme PR Best regards, Krist

…

-- Krist Wongsuphasawat http://kristw.yellowpigz.com

On Sep 20, 2018, 08:27 -0700, Maxime Beauchemin ***@***.***>, wrote: @kristw @williaster I'm not quite sure why, but since this PR it looks like we've been running only a handful of javascript tests. codecov-io is often out of whack, but it looks like it was in tune here detecting this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kristw · 2018-09-20T15:56:24Z

I can create the fix as a separate PR too. Apologize for the issue. Best regards, Krist

…

-- Krist Wongsuphasawat http://kristw.yellowpigz.com

On Sep 20, 2018, 08:27 -0700, Maxime Beauchemin ***@***.***>, wrote: @kristw @williaster I'm not quite sure why, but since this PR it looks like we've been running only a handful of javascript tests. codecov-io is often out of whack, but it looks like it was in tune here detecting this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

…structure. (apache#5893) * reactify world map * add createAdaptor * fix typo * add schema * move directory * remove unnecessary lines * make setRef a function * convert keys to camelcase * add unit test * update formatting * add check for displayName * pass width and height as separate inputs

kristw mentioned this pull request Sep 14, 2018

[SIP-6] [Embeddable Charts] Migrate visualization-specific code to js plugin #5692

Closed

kristw closed this Sep 14, 2018

kristw reopened this Sep 14, 2018

williaster reviewed Sep 17, 2018

View reviewed changes

conglei reviewed Sep 17, 2018

View reviewed changes

kristw added 12 commits September 18, 2018 12:14

reactify world map

d503b46

add createAdaptor

f0685db

fix typo

37092de

add schema

2cf2863

move directory

b152eb5

remove unnecessary lines

88c41b3

make setRef a function

d2bf050

convert keys to camelcase

c2457e8

add unit test

c4cc4b5

update formatting

dbe5833

add check for displayName

bafb382

pass width and height as separate inputs

86ee56c

kristw force-pushed the kristw-reactify branch from 122076f to 86ee56c Compare September 18, 2018 19:31

kristw closed this Sep 18, 2018

kristw reopened this Sep 18, 2018

williaster approved these changes Sep 19, 2018

View reviewed changes

williaster merged commit 8fff0d9 into apache:master Sep 19, 2018

kristw deleted the kristw-reactify branch September 19, 2018 15:45

This was referenced Sep 21, 2018

[SIP-6] Migrate visualizations to new directory structure. #5949

Merged

[SIP-6] Migrate visualizations to new directory structure (part 2) #5997

Merged

[SIP-6] Migrate visualizations to new directory structure (part 3 - nvd3) #6006

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIP-6] Add reactify function and convert world map to new directory structure. #5893

[SIP-6] Add reactify function and convert world map to new directory structure. #5893

kristw commented Sep 14, 2018 •

edited

Loading

williaster left a comment

williaster Sep 17, 2018

williaster Sep 17, 2018

kristw Sep 17, 2018

williaster Sep 17, 2018

kristw Sep 17, 2018

williaster Sep 17, 2018

kristw Sep 17, 2018

kristw Sep 18, 2018

williaster Sep 19, 2018

conglei Sep 17, 2018

conglei Sep 17, 2018

kristw Sep 17, 2018

williaster Sep 17, 2018

conglei Sep 17, 2018

williaster Sep 17, 2018

kristw Sep 17, 2018 •

edited

Loading

conglei Sep 17, 2018

williaster Sep 17, 2018

kristw Sep 17, 2018

codecov-io commented Sep 18, 2018

codecov-io commented Sep 18, 2018

codecov-io commented Sep 18, 2018

kristw commented Sep 18, 2018

williaster left a comment

mistercrunch commented Sep 20, 2018 •

edited

Loading

kristw commented Sep 20, 2018 via email

kristw commented Sep 20, 2018 via email

		@@ -0,0 +1,11 @@
		import { mapKeys, camelCase, isPlainObject } from 'lodash/fp';

		export default function convertKeysToCamelCase(object) {

		@@ -0,0 +1,5 @@
		import createAdaptor from '../../utils/createAdaptor';

		@@ -0,0 +1,11 @@
		export default function transformProps(basicChartInput) {

[SIP-6] Add reactify function and convert world map to new directory structure. #5893

[SIP-6] Add reactify function and convert world map to new directory structure. #5893

Conversation

kristw commented Sep 14, 2018 • edited Loading

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

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 Sep 17, 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

codecov-io commented Sep 18, 2018

Codecov Report

codecov-io commented Sep 18, 2018

Codecov Report

codecov-io commented Sep 18, 2018

Codecov Report

kristw commented Sep 18, 2018

williaster left a comment

Choose a reason for hiding this comment

mistercrunch commented Sep 20, 2018 • edited Loading

kristw commented Sep 20, 2018 via email

kristw commented Sep 20, 2018 via email

kristw commented Sep 14, 2018 •

edited

Loading

kristw Sep 17, 2018 •

edited

Loading

mistercrunch commented Sep 20, 2018 •

edited

Loading