-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Changes from all commits
d503b46
f0685db
37092de
2cf2863
b152eb5
88c41b3
d2bf050
c2457e8
c4cc4b5
dbe5833
bafb382
86ee56c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||
}); | ||
}); |
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) { | ||
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}`); | ||
} |
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), | ||
); | ||
}; | ||
} |
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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we also check for + add if (renderFn.displayName) {
ReactifiedComponent.displayName = renderFn.displayName;
} |
||
return ReactifiedComponent; | ||
} |
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you think about just using My main thought is that this might make naming more consistent with visualizations that are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am afraid I will make mistake when importing. If I specify There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good point. I can definitely rename it to |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import createAdaptor from '../../utils/createAdaptor'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think including it in |
||
import WorldMap from './ReactWorldMap'; | ||
import transformProps from './transformProps'; | ||
|
||
export default createAdaptor(WorldMap, transformProps); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export default function transformProps(basicChartInput) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we could do some propType check on formData. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be part of the For example, 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, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
[VIZ_TYPES.dual_line]: loadNvd3, | ||
[VIZ_TYPES.event_flow]: () => | ||
loadVis(import(/* webpackChunkName: "EventFlow" */ './EventFlow.jsx')), | ||
|
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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7487,6 +7487,10 @@ [email protected], 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" | ||
|
||
[email protected], log-symbols@^2.2.0: | ||
version "2.2.0" | ||
resolved "https://registry.yarnpkg.com/log-symbols/-/log-symbols-2.2.0.tgz#5740e1c5d6f0dfda4ad9323b5332107ef6b4c40a" | ||
|
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.
lovely!