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(discover): Add to Dashboard default Widget Display Type based on Discover Query #29518

Merged

Conversation

edwardgou-sentry
Copy link
Contributor

Updates the Discover Add to Dashboard modal to default the Display Type based on the Display Mode of the saved query. ie if the Discover query is in World Map view, clicking Add to Dashboard will default the widget to World Map type.

@edwardgou-sentry edwardgou-sentry requested a review from a team October 22, 2021 17:02
@edwardgou-sentry edwardgou-sentry requested a review from a team as a code owner October 22, 2021 17:02
Copy link
Member

@shruthilayaj shruthilayaj left a comment

Choose a reason for hiding this comment

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

LGTM, can we add top events to this too or is that follow up work?

if (!widget) {
this.state = {
title: defaultTitle ?? '',
displayType: DisplayType.LINE,
displayType: defaultDisplayType ?? DisplayType.LINE,
Copy link
Member

Choose a reason for hiding this comment

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

nittiest nit: can we name this prop displayType instead of defaultDisplayType? because technically DisplayType.LINE is the default 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updated!

@github-actions
Copy link
Contributor

size-limit report

Path Base Size (f6aad2c) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.74 KB 52.74 KB +0.01% 🔺
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.89 KB 70.89 KB 0%

@edwardgou-sentry
Copy link
Contributor Author

LGTM, can we add top events to this too or is that follow up work?

Ahh good call on the Top Events, forgot about that.
I think I will do this one in a follow up PR. The changes are a little bit more than just adding Top events to the mapping (I'll have to update the modal to pull the columns from the query in order for the top events to display correctly on the widget)

@edwardgou-sentry edwardgou-sentry merged commit 478b677 into master Oct 22, 2021
@edwardgou-sentry edwardgou-sentry deleted the feat/discover-add-to-dashboard-use-display-type branch October 22, 2021 18:10
@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants