Skip to content
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

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

Merged
merged 12 commits into from
Sep 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset/assets/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
"jed": "^1.1.1",
"jquery": "3.1.1",
"json-bigint": "^0.3.0",
"lodash": "^4.17.11",
"lodash.throttle": "^4.1.1",
"mapbox-gl": "^0.45.0",
"mathjs": "^3.20.2",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { it, describe } from 'mocha';
import { expect } from 'chai';
import convertKeysToCamelCase from '../../../src/utils/convertKeysToCamelCase';

describe.only('convertKeysToCamelCase(object)', () => {
it('returns undefined for undefined input', () => {
expect(convertKeysToCamelCase(undefined)).to.equal(undefined);
});
it('returns null for null input', () => {
expect(convertKeysToCamelCase(null)).to.equal(null);
});
it('returns a new object that has all keys in camelCase', () => {
const input = {
is_happy: true,
'is-angry': false,
isHungry: false,
};
expect(convertKeysToCamelCase(input)).to.deep.equal({
isHappy: true,
isAngry: false,
isHungry: false,
});
});
it('throws error if input is not a plain object', () => {
expect(() => { convertKeysToCamelCase({}); }).to.not.throw();
expect(() => { convertKeysToCamelCase(''); }).to.throw();
expect(() => { convertKeysToCamelCase(new Map()); }).to.throw();
});
});
11 changes: 11 additions & 0 deletions superset/assets/src/utils/convertKeysToCamelCase.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { mapKeys, camelCase, isPlainObject } from 'lodash/fp';

export default function convertKeysToCamelCase(object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lovely!

if (object === null || object === undefined) {
return object;
}
if (isPlainObject(object)) {
return mapKeys(k => camelCase(k), object);
}
throw new Error(`Cannot convert input that is not a plain object: ${object}`);
}
19 changes: 19 additions & 0 deletions superset/assets/src/utils/createAdaptor.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from 'react';
import ReactDOM from 'react-dom';
import BasicChartInput from '../visualizations/models/BasicChartInput';

const IDENTITY = x => x;

export default function createAdaptor(Component, transformProps = IDENTITY) {
return function adaptor(slice, payload, setControlValue) {
const basicChartInput = new BasicChartInput(slice, payload, setControlValue);
ReactDOM.render(
<Component
width={slice.width()}
height={slice.height()}
{...transformProps(basicChartInput)}
/>,
document.querySelector(slice.selector),
);
};
}
54 changes: 54 additions & 0 deletions superset/assets/src/utils/reactify.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import React from 'react';

export default function reactify(renderFn) {
class ReactifiedComponent extends React.Component {
constructor(props) {
super(props);
this.setContainerRef = this.setContainerRef.bind(this);
}

componentDidMount() {
this.execute();
}

componentDidUpdate() {
this.execute();
}

componentWillUnmount() {
this.container = null;
}

setContainerRef(c) {
this.container = c;
}

execute() {
if (this.container) {
renderFn(this.container, this.props);
}
}

render() {
const { id, className } = this.props;
return (
<div
id={id}
className={className}
ref={this.setContainerRef}
/>
);
}
}

if (renderFn.displayName) {
ReactifiedComponent.displayName = renderFn.displayName;
}
if (renderFn.propTypes) {
ReactifiedComponent.propTypes = renderFn.propTypes;
}
if (renderFn.defaultProps) {
ReactifiedComponent.defaultProps = renderFn.defaultProps;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we also check for + add displayName?

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

return ReactifiedComponent;
}
4 changes: 4 additions & 0 deletions superset/assets/src/visualizations/WorldMap/ReactWorldMap.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import reactify from '../../utils/reactify';
import WorldMap from './WorldMap';

export default reactify(WorldMap);
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import d3 from 'd3';
import PropTypes from 'prop-types';
import Datamap from 'datamaps';
import './world_map.css';
import './WorldMap.css';

const propTypes = {
data: PropTypes.arrayOf(PropTypes.shape({
Expand Down Expand Up @@ -109,20 +109,4 @@ function WorldMap(element, props) {

WorldMap.propTypes = propTypes;

function adaptor(slice, payload) {
const { selector, formData } = slice;
const {
max_bubble_size: maxBubbleSize,
show_bubbles: showBubbles,
} = formData;
const element = document.querySelector(selector);

return WorldMap(element, {
data: payload.data,
height: slice.height(),
maxBubbleSize: parseInt(maxBubbleSize, 10),
showBubbles,
});
}

export default adaptor;
export default WorldMap;
5 changes: 5 additions & 0 deletions superset/assets/src/visualizations/WorldMap/adaptor.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import createAdaptor from '../../utils/createAdaptor';
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

import WorldMap from './ReactWorldMap';
import transformProps from './transformProps';

export default createAdaptor(WorldMap, transformProps);
10 changes: 10 additions & 0 deletions superset/assets/src/visualizations/WorldMap/transformProps.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export default function transformProps(basicChartInput) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could do some propType check on formData.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@kristw kristw Sep 17, 2018

Choose a reason for hiding this comment

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

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.

const { formData, payload } = basicChartInput;
const { maxBubbleSize, showBubbles } = formData;

return {
data: payload.data,
maxBubbleSize: parseInt(maxBubbleSize, 10),
showBubbles,
};
}
2 changes: 1 addition & 1 deletion superset/assets/src/visualizations/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

[VIZ_TYPES.dual_line]: loadNvd3,
[VIZ_TYPES.event_flow]: () =>
loadVis(import(/* webpackChunkName: "EventFlow" */ './EventFlow.jsx')),
Expand Down
10 changes: 10 additions & 0 deletions superset/assets/src/visualizations/models/BasicChartInput.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import convertKeysToCamelCase from '../../utils/convertKeysToCamelCase';

export default class BasicChartInput {
constructor(slice, payload, setControlValue) {
this.annotationData = slice.annotationData;
this.formData = convertKeysToCamelCase(slice.formData);
this.payload = payload;
this.setControlValue = setControlValue;
}
}
4 changes: 4 additions & 0 deletions superset/assets/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -7487,6 +7487,10 @@ lodash@4.17.10, lodash@^4.0.1, lodash@^4.0.8, lodash@^4.13.1, lodash@^4.14.0, lo
version "4.17.10"
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.10.tgz#1b7793cf7259ea38fb3661d4d38b3260af8ae4e7"

lodash@^4.17.11:
version "4.17.11"
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.11.tgz#b39ea6229ef607ecd89e2c8df12536891cac9b8d"

log-symbols@2.2.0, log-symbols@^2.2.0:
version "2.2.0"
resolved "https://registry.yarnpkg.com/log-symbols/-/log-symbols-2.2.0.tgz#5740e1c5d6f0dfda4ad9323b5332107ef6b4c40a"
Expand Down