-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(issues): Add details to current event marker tooltip #106204
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
Conversation
On the line that indicates the current event, add all the tooltips from the regular tooltip so that the tooltip doesn't feel in the way. Also fix a color issue. Co-Authored-By: Claude <noreply@anthropic.com>
|
|
||
| // Colors matching eventGraph.tsx bar chart series | ||
| const translucentGray = Color(theme.colors.gray400).alpha(0.5).string(); | ||
| const lightGray = Color(theme.colors.gray400).alpha(0.2).string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tooltip marker colors don't match bar chart colors
Medium Severity
The tooltip marker colors are hardcoded as gray400 with alpha values and blue400, but the bar chart in eventGraph.tsx uses theme.tokens.dataviz.semantic.other, theme.tokens.dataviz.semantic.accent, and theme.tokens.dataviz.semantic.neutral. The comment claims "Colors matching eventGraph.tsx bar chart series" but these are different color sources, causing the tooltip dots to visually mismatch the actual bar colors they represent.
Additional Locations (1)
| event, | ||
| group, | ||
| eventSeries, | ||
| isFiltered: isUnfilteredStatsEnabled, | ||
| unfilteredEventSeries, | ||
| seriesType: visibleSeries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The event graph's current event marker tooltip displays event counts instead of user counts when the user graph is active, because it's always passed the eventSeries data.
Severity: HIGH
Suggested Fix
Conditionally pass the correct data series to the useCurrentEventMarklineSeries hook. When visibleSeries is 'user', the hook should receive userSeries and unfilteredUserSeries instead of eventSeries and unfilteredEventSeries.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/views/issueDetails/streamline/eventGraph.tsx#L244-L249
Potential issue: In the event graph, when the displayed series is switched to 'user',
the tooltip for the current event marker incorrectly shows event counts instead of user
counts. The `useCurrentEventMarklineSeries` hook is always called with `eventSeries`
(event counts), but the tooltip labels change to 'Matching users' and 'Total users'.
This results in a data mismatch, where the UI displays event count values next to user
count labels, potentially misleading the user about the data they are viewing.
Did we get this right? 👍 / 👎 to inform future reviews.
JonasBa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on a fix for the tokens, we have a backwards compat proxy here that doesn't seem to be applied correctly when this is not accessed from a string literal template
On the line that indicates the current event, add all the tooltips from the regular tooltip so that the tooltip doesn't feel in the way. Also fix a color issue
before
after