feat(err): enhance sparkline component#30236
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR enhances the sparkline visualization component with timeseries support, hover states, and customizable tooltips, while consolidating sparkline functionality across error tracking features.
Key changes:
- Introduces new
OccurenceSparklinecomponent replacingSparklinePanelwith improved timeseries visualization - Adds dark mode support and proper time unit formatting in sparkline tooltips
- Implements
useSparklineDatahook for centralized data handling and aggregation - Refactors scale configuration into separate objects for better maintainability
- Simplifies
sparklineLabelsutility to allow flexible date formatting at UI layer
The changes appear well-structured and improve code organization while adding valuable features to the sparkline visualization system.
7 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
|
Size Change: -21 B (0%) Total Size: 9.75 MB ℹ️ View Unchanged
|
daibhin
left a comment
daibhin
left a comment
There was a problem hiding this comment.
I pushed a PR to fix some small hiccups and the typing of ScaleOptions. Left a couple of questions that we can go through tomorrow if you like
frontend/src/scenes/error-tracking/issue/OccurenceSparkline.tsx
Outdated
Show resolved
Hide resolved
| return useMemo(() => { | ||
| return { | ||
| color: isDarkModeOn ? 'primitive-neutral-200' : 'primitive-neutral-800', | ||
| hoverColor: 'primary-3000', |
There was a problem hiding this comment.
I'm not sure about the transition from the dark grey to the orange. It's very noticeable and looks laggy because it's going across the colour wheel. What do you think about a lighter grey that transitions to a darker grey for the highlighted line?
There was a problem hiding this comment.
I tried it with only few data points, it might be too eye-catching with more. Let's try with variant of gray
|
|
||
| export const Metadata = (): JSX.Element => { | ||
| const { issue, issueLoading } = useValues(errorTrackingIssueSceneLogic) | ||
| const [values, unit, interval] = useSparklineData(issue?.aggregations) |
There was a problem hiding this comment.
The useSparklineData relies on the sparklineSelectedPeriod hook internally, which is configured on the list page. This means the sparkline could appear different for different customers on the issue page
There was a problem hiding this comment.
The date range on the issue page comes from the listing, so in any case the sparkline will be different for two users, right? (if one comes from listing and the other one directly from issue url).
| }, [isDarkModeOn]) | ||
| } | ||
|
|
||
| export function useSparklineData(aggregations?: ErrorTrackingIssueAggregations): [number[], TimeUnit, number] { |
There was a problem hiding this comment.
Should this be taking an optional type? I think it's making the implementation of the component more complicated and causing us to do checks to see if there are any returned values in other components
| export function useSparklineData(aggregations?: ErrorTrackingIssueAggregations): [number[], TimeUnit, number] { | |
| export function useSparklineData(aggregations: ErrorTrackingIssueAggregations): [number[], TimeUnit, number] { |
There was a problem hiding this comment.
I think keeping this arg optional will make component implementation easier because you don't have to check if aggregation is populated before using this hook. Values should always be valid for sparkline component, no need to check, I removed the condition here
|
Oh, one other thing. I can be good to include a gif / screenshot for visual changes like this. It can help to review without needing to pull the branch sometimes |
| hoverColor?: string | ||
| } | ||
|
|
||
| export type AnyScaleOptions = ScaleOptions<'linear' | 'logarithmic' | 'time' | 'timeseries' | 'category'> |
Co-authored-by: David Newell <d.newell1@outlook.com>
hpouillot
force-pushed
the
err/sparkline-refresh
branch
from
167ec48 to
c0ac413
Compare
3 participants
Changes
- display X axis as timeserie
- add hover state on data
- ability to customize tooltip label