Flux/send query to dashboard #4229
Conversation
AlirieGray
force-pushed
the
flux/send-query-to-dashboard
branch
from
ef268fe to
8dda5e3
Compare
AlirieGray
force-pushed
the
flux/send-query-to-dashboard
branch
3 times, most recently
from
f040b38 to
94c5578
Compare
| class SendToDashboardOverlay extends PureComponent<Props, State> { | ||
| constructor(props) { | ||
| super(props) | ||
| const {queryConfig} = this.props |
There was a problem hiding this comment.
(nitpick) how about const {queryConfig} = props instead? it would be more consistent with the rest of the codebase
|
|
||
| this.state = { | ||
| selectedIDs: [], | ||
| hasQuery: queryConfig.fields.length !== 0, |
There was a problem hiding this comment.
since fields is optional on a QueryConfig, it would be good to guard this property access with
hasQuery: queryConfig.fields && queryConfig.fields.length !== 0
There was a problem hiding this comment.
or via getDeep, if that's your style
There was a problem hiding this comment.
we could also just check if rawText (the query) is an empty string or if queryConfig is null. thoughts?
| <OverlayHeading title="Send to Dashboard" /> | ||
| <OverlayBody> | ||
| <div className="form-group"> | ||
| <label htmlFor="New Cell Name"> New Cell Name </label> |
There was a problem hiding this comment.
had never seen for on a label before, that's cool. is it for screen readers?
There was a problem hiding this comment.
I think it is, yeah
| private get dropdownItems(): JSX.Element[] { | ||
| const {dashboards} = this.props | ||
|
|
||
| const simpleArray = dashboards.map(d => ({ |
There was a problem hiding this comment.
(nitpick) this might read a bit easier if it's done all in one pass
return dashboards.map(dashboard => {
const id = dashboard.id.toString()
return (
<MultiSelectDropdown.Item key={id} id={id} value={id}>
{dashboard.name}
</MultiSelectDropdown.Item>
)
})
| const {dashboards} = this.props | ||
| const {selectedIDs} = this.state | ||
|
|
||
| return dashboards.filter(d => { |
There was a problem hiding this comment.
(nitpick) I think this can be done in one line with dashboards.filter(d => selectedIDs.includes(d.id.toString()))
There was a problem hiding this comment.
I like it! very clean
| const {query} = qs.parse(location.search, {ignoreQueryPrefix: true}) | ||
| await handleGetDashboards() |
There was a problem hiding this comment.
before making this request, could you check if dashboards are already in the redux store? it would be great to avoid making this request unless we absolutely have to, since it's quite expensive (it loads every cell for every dashboard as well).
| <GraphTips /> | ||
| <div | ||
| className="btn btn-sm btn-default" | ||
| <button |
There was a problem hiding this comment.
there's a great Button reusable ui component that you could use here
| @@ -231,6 +256,12 @@ export class DataExplorer extends PureComponent<Props, State> { | |||
| </> | |||
| ) | |||
| } | |||
|
|
|||
| private toggleSendToDashboard = () => { | |||
There was a problem hiding this comment.
(nitpick) how about toggleIsSendToDashboardVisible instead? seems more explicit
| @@ -265,6 +298,8 @@ const mapDispatchToProps = dispatch => { | |||
| dataExplorerActionCreators, | |||
| dispatch | |||
| ), | |||
| handleGetDashboards: bindActionCreators(getDashboardsAsync, dispatch), | |||
There was a problem hiding this comment.
(nitpick) this isn't consistent across the codebase (not even used here) but I've been digging the following naming convention lately:
- callbacks passed as props are named like
onFoo - callbacks defined and used within the same class are named like
handleFoo
so in this case it would be onHandleGetDashboards and onAddDashboardCell
There was a problem hiding this comment.
Looks great.
I had a bunch of pedantic style-related suggestions while reading through. Please feel free to ignore these types of comments if they seem unreasonable.
I did pull down the branch and check it out. Is there a design for this overlay?
Is it going to get styled in this PR or another?
AlirieGray
force-pushed
the
flux/send-query-to-dashboard
branch
6 times, most recently
from
4dc8ec8 to
77b013d
Compare
Co-authored-by: Alex Paxton <[email protected]>
Disable button if cell name is empty. Co-authored-by: Alex Paxton <[email protected]>
AlirieGray
force-pushed
the
flux/send-query-to-dashboard
branch
from
77b013d to
ad9b9c8
Compare
Closes #4142
What was the problem?
The user experience of the Data Explorer and the Cell Editor Overlay was similar but not consistent, so that users were using the Data Explorer to create queries but then had to switch to the Dashboards page in order to put that query into a cell.
What was the solution?
These changes allow the user to create a query and press a button to create a cell with that query in one or more dashboards.