-
Notifications
You must be signed in to change notification settings - Fork 13.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
[explore] improve metric(s) and groupby(s) controls #2921
Conversation
mistercrunch
commented
Jun 7, 2017
- surface verbose_name, description & expression in controls
- [table viz] surface verbose name in table header
@@ -75,6 +75,7 @@ class ChartContainer extends React.PureComponent { | |||
return { | |||
viewSqlQuery: this.props.queryResponse.query, | |||
containerId: props.containerId, | |||
datasource: props.datasource, |
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 why we are using this.props
and props
in this method. would be nice to clean this up and use either props
or this.props
for consistency, obvs optional.
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 catch, should be consistent and props.
all around
@@ -27,6 +31,9 @@ const defaultProps = { | |||
multi: false, | |||
onChange: () => {}, | |||
showHeader: true, | |||
optionRenderer: opt => opt.label, | |||
valueRenderer: opt => opt.label, |
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 this be valueRenderer: opt => opt.value
?
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 bit confusing but essentially the optionRenderer
applies to what's in the dropdown and the valueRenderer
applies to the selected item(s). It's nice to have this level of control, for instance we could show the expression
behind the metric only on the selected metric, not in the list if we wanted to.
So they both should show the label.
@@ -18,6 +19,48 @@ const ROW_LIMIT_OPTIONS = [10, 50, 100, 250, 500, 1000, 5000, 10000, 50000]; | |||
|
|||
const SERIES_LIMITS = [0, 5, 10, 25, 50, 100, 500]; | |||
|
|||
const metricRenderer = m => ( |
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.
ideally components shouldn't live in a stores file. these should probably be their own component files so they can be used in other places as needed.
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.
Agreed, I thought about it when starting to write it, especially makes sense as it is fairly large. I'll address that.
- surface verbose_name, description & expression in controls - [table viz] surface verbose name in table header
LGTM! |