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.

Sign up for GitHub

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

Jump to bottom

ref(echart-tooltip): Add indent hack #29077

Merged
merged 1 commit into from
Oct 7, 2021
Merged

ref(echart-tooltip): Add indent hack #29077

merged 1 commit into from
Oct 7, 2021

Conversation

priscilawebdev
Copy link
Member

@priscilawebdev priscilawebdev commented Oct 5, 2021 •
edited
Loading

@priscilawebdev priscilawebdev requested a review from a team as a code owner October 5, 2021 15:56
k-fish
k-fish reviewed Oct 5, 2021
View reviewed changes
Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Rather then rely on modifying the seriesName and using a string hack, I think instead you should just build an object for additional tooltip options for each series name, then pass that to your chart in the tooltip param -> that gets passed as props to Tooltip -> you can use that inside the formatter.

@priscilawebdev priscilawebdev force-pushed the ref/echart-tooltip-add-indent-hack branch from 0707963 to febf139 Compare October 6, 2021 13:34
@priscilawebdev priscilawebdev requested a review from k-fish October 6, 2021 13:41
markstory
markstory approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me. Using the series names to indent is a simple interface, and preferrable to using series indexes imo.

k-fish
k-fish approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Yeah, this looks better 👍

@priscilawebdev priscilawebdev force-pushed the ref/echart-tooltip-add-indent-hack branch from febf139 to 316a289 Compare October 7, 2021 06:42
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2021

size-limit report

Path Base Size (720676c) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.67 KB 52.67 KB -0.01% 🔽
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.9 KB 70.9 KB 0%

@priscilawebdev priscilawebdev enabled auto-merge (squash) October 7, 2021 07:47
@priscilawebdev priscilawebdev merged commit 2bb85c6 into master Oct 7, 2021
@priscilawebdev priscilawebdev deleted the ref/echart-tooltip-add-indent-hack branch October 7, 2021 07:56
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@markstory markstory markstory approved these changes

@k-fish k-fish k-fish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@priscilawebdev @markstory @k-fish