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): enable multi y axis for previous period view #28951

Merged
merged 13 commits into from
Oct 8, 2021

Conversation

edwardgou-sentry
Copy link
Contributor

@edwardgou-sentry edwardgou-sentry commented Sep 29, 2021

Adds multi y axis support for Previous Period display view in Discover results chart:
image

When using multiple y axis series, previous period series will be plotted as stacked dotted lines using colours based off the original series. Single y axis previous period display is unchanged (will still use grey when plotting previous period series).

currentSeriesName and previousSeriesName props in EventsRequest were pluralized and changed from string type to string[] to support multiple previous series. Some dependants of EventsRequest were updated to reflect this but without any functional changes.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2021

size-limit report

Path Base Size (284fefe) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.66 KB 52.67 KB +0.02% 🔺
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.89 KB 70.9 KB +0.03% 🔺

@edwardgou-sentry edwardgou-sentry changed the title feat(discover): enable multi y axis for previous period feat(discover): enable multi y axis for previous period view Oct 1, 2021
@edwardgou-sentry
Copy link
Contributor Author

edwardgou-sentry commented Oct 1, 2021

size-limit report

Path Base Size (493559e) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.51 KB 52.51 KB -0.02% 🔽
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.89 KB 70.89 KB 0%

Is this a false alarm?

@edwardgou-sentry edwardgou-sentry requested review from a team October 1, 2021 14:36
@edwardgou-sentry edwardgou-sentry marked this pull request as ready for review October 1, 2021 14:37
@edwardgou-sentry edwardgou-sentry requested a review from a team as a code owner October 1, 2021 14:37
@edwardgou-sentry edwardgou-sentry requested a review from a team October 1, 2021 14:37
@edwardgou-sentry edwardgou-sentry requested a review from a team as a code owner October 1, 2021 14:37
@edwardgou-sentry edwardgou-sentry requested a review from a team October 1, 2021 14:37
@scttcper
Copy link
Member

scttcper commented Oct 1, 2021

Is this a false alarm?

I think baseChart is in the main bundle

Copy link
Member

@wmak wmak left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, but I think this might have broken previous period legends on single axis

image

And isn't formatting equations properly when multi-axis is enabled:

image

@wmak wmak requested a review from a team October 4, 2021 23:05
@edwardgou-sentry
Copy link
Contributor Author

@wmak 👍 nice catch, fixed!

@edwardgou-sentry edwardgou-sentry requested a review from wmak October 5, 2021 15:29
@edwardgou-sentry edwardgou-sentry requested a review from a team October 7, 2021 18:15
Copy link
Member

@wmak wmak left a comment

Choose a reason for hiding this comment

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

LGTM

@edwardgou-sentry edwardgou-sentry merged commit 0ca66b6 into master Oct 8, 2021
@edwardgou-sentry edwardgou-sentry deleted the feat/discover-multi-y-axis-previous-period branch October 8, 2021 19:33
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 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.

3 participants