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

feat(dashboards): add overlay to create dashboard from template #13078

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

AlirieGray
Copy link
Contributor

@AlirieGray AlirieGray commented Apr 2, 2019

Closes #12913

This PR adds an overlay to create a new dashboard from a template.

2019-04-01 17 22 52

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Rebased/mergeable
  • Tests pass

@AlirieGray AlirieGray force-pushed the templates/import-resource-from-template-overlay branch from 35b8625 to 6cde9ac Compare April 2, 2019 00:26
router,
params: {orgID},
} = this.props
router.push(`/orgs/${orgID}/dashboards/templates`)
Copy link
Contributor

Choose a reason for hiding this comment

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

this route url should have something in it to indicate that its import /orgs/${orgID}/dashboards/import/template would be my go to.

title={_.get(selectedTemplateSummary, 'meta.name')}
/>
<Panel.Body>
{/* TODO: Add Template description */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not leave comments in JSX

@@ -1,5 +1,6 @@
// Libraries
import {Dispatch} from 'redux'
import _ from 'lodash'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need lodash here? Nothing else added seems to use it.

</ResponsiveGridSizer>
</OrgTemplateFetcher>
{!selectedTemplateSummary && (
<Panel className="import-template-overlay--empty">
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 extract the empty state? There is a lot going on in this render function.

@@ -117,6 +118,11 @@ class Dashboards extends PureComponent<Props, State> {
router.push(`/organizations/${params.orgID}/dashboards/import`)
}

private summonImportFromTemplateOverlay = (): void => {
const {router, params} = this.props
router.push(`/organizations/${params.orgID}/dashboards/templates`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about the route.

<Dropdown.Item id={newOption} key={newOption} value={newOption}>
{newOption}
</Dropdown.Item>,
<Dropdown.Item id={importOption} key={importOption} value={importOption}>
{importOption}
</Dropdown.Item>,
]

if (!!this.props.onSelectTemplate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd that the existence of a handler is determining whether to render this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would you recommend a boolean prop instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that seems more informative

Copy link
Contributor

@bthesorceror bthesorceror left a comment

Choose a reason for hiding this comment

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

A few things to fix but otherwise 🍰

@AlirieGray AlirieGray force-pushed the templates/import-resource-from-template-overlay branch 5 times, most recently from 44849b1 to 874bc16 Compare April 2, 2019 20:46
@AlirieGray AlirieGray force-pushed the templates/import-resource-from-template-overlay branch 3 times, most recently from 965a598 to c124273 Compare April 2, 2019 21:21
@AlirieGray AlirieGray force-pushed the templates/import-resource-from-template-overlay branch from c124273 to d10d216 Compare April 2, 2019 21:33
@AlirieGray AlirieGray merged commit e1b9ddb into master Apr 2, 2019
@AlirieGray AlirieGray deleted the templates/import-resource-from-template-overlay branch April 2, 2019 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants