-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
[explorev2] chart and controls #1251
Conversation
* create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log
* Created store and reducers * Added spec * Modifications based on comments
@@ -0,0 +1,175 @@ | |||
const { assign } = Object; |
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.
unrelated question, if we are not using babel-polyfill
are we relying on Object.assign
coming from chrome / the browser?
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.
we use babel and these presets so Object.assign is polyfilled.
"babel": "^6.3.26",
"babel-core": "^6.10.4",
"babel-loader": "^6.2.4",
"babel-preset-airbnb": "^2.0.0",
"babel-preset-react": "^6.11.1",
data: props.viz.data, | ||
height: window.innerHeight, | ||
label1: 'Label 1', | ||
}; |
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.
should you add this.getParamsFromUrl = this.getParamsFromUrl.bind(this)
+ same for formatDates
?
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.
since getParamsFromUrl
and formatDates
don't need access to this
we don't need to bind this
to the methods. typically we would use bind(this)
if we were passing a method to an event callback that needed access to the components context.
} | ||
|
||
getParamsFromUrl() { | ||
const hash = window.location.search; |
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.
curious if we've thought about using react router for url state management? I used it in dataportal and liked it.
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.
this is a good suggestion... let's keep it in mind as we continue the refactor
formatDates(values) { | ||
const newValues = values.map((val) => { | ||
return { | ||
x: moment(new Date(val.x)).format('MMM D'), |
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.
FYI d3 also has really good date formatting utilities, not sure if it'd be more lightweight than moment.
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.
yeah, i'm not sure either.. let's use moment for now and we can change to use d3 time utils in the future if that makes sense
|
||
getHeight() { | ||
const navHeight = 90; | ||
return (window.innerHeight - navHeight) + 'px'; |
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.
do we use template literals anywhere or is this just a preference you have?
${window.innerHeight - navHeight}px
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.
template literals are nice, will change. mostly just old habit here :)
export default class TimeSeriesLineChart extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
this.keysToColorsMap = this.mapKeysToColors(props.data); |
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.
similar question about binding component methods
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.
methods only need to be bound in certain cases, first if the method needs access to this
and depending on where the function is getting called, ie. in a callback after an event is triggered.
} | ||
|
||
mapKeysToColors(data) { | ||
// todo: what if there are more lines than colors in schemeCategory20c? |
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.
I believe that d3 performs i % lengthOfCategoryArray
so that it just re-uses colors. still annoying that colors are re-used, need magic from @elibrumbaugh
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.
also it might be worth checking with @mistercrunch about how he does colors currently. when there are multiple charts on a dashboard and they share some categories, ideally you do not assign/use two different colors for the same category on different charts. at one point he was hashing keys to always get the same color. not that trivial...
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.
good points here. i'll make an issue to track this.
keysToColorsMap: PropTypes.object.isRequired, | ||
}; | ||
|
||
export default class Legend extends React.Component { |
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.
why not a pure function here?
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.
good point, will simplify this
} | ||
|
||
render() { | ||
const legendEls = this.props.data.map((d) => { |
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.
eventually it'd be cool to make the legend expandable in the case that there are many series, then it wouldn't dominate most of the real estate in a dashboard chart
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.
+1
love the lack of grid lines 👯♂️ 👯 !!! |
thanks for the review @williaster! made some changes. going to merge to master once tests are passing. |
need to fix these linting errors: /home/travis/build/airbnb/caravel/caravel/assets/javascripts/explorev2/components/ChartContainer.jsx
/home/travis/build/airbnb/caravel/caravel/assets/javascripts/explorev2/components/charts/TimeSeriesLineChart.jsx
/home/travis/build/airbnb/caravel/caravel/assets/javascripts/explorev2/components/ExploreViewContainer.jsx
|
@ascott You mentioned that the visualizations should be converted to react. Is the plan to replace in the basic charts NVD3 with Victory? Do you plan to wrap the visualizations that use only D3 into react components? Switching from exploreV1 to exploreV2 gives the chance to implement faceting (e.g. multiple scatter plots in one viz, one for (a combination of) categorical variable(s) ). Example. Because the the subplots should use the same scaling for the data, there has to be some dependency between them. |
to view explore v2 in progress, change url from
explore
toexploreV2
http://localhost:8081/caravel/exploreV2/table/3/?{...}
explore-v1
![screenshot 2016-10-04 10 35 05](https://cloud.githubusercontent.com/assets/130878/19085224/6d83633e-8a1e-11e6-8164-50f11f7998a4.png)
explore-v2
![screenshot 2016-10-05 11 30 20](https://cloud.githubusercontent.com/assets/130878/19126313/40e92fd0-8aef-11e6-8a85-0231a5bb11f9.png)
@vera-liu @bkyryliuk @mistercrunch @williaster