ref(groupChart): Refactor GroupChart for readability, typing, and scoping #70966
Conversation
github-actions
bot
added
the
Scope: Frontend
| data: Group; | ||
| statsPeriod: string; | ||
| stats: ReadonlyArray<TimeseriesValue>; | ||
| height?: number; | ||
| secondaryStats?: ReadonlyArray<TimeseriesValue>; | ||
| showMarkLine?: boolean; | ||
| showSecondaryPoints?: boolean; |
There was a problem hiding this comment.
The statsPeriod props was only being used to extract the correct series from the group prop. The GroupChart component shouldn't care about the group itself or which time period is being displayed, it should only care about and receive the actual series data itself, hence why the stats and secondaryStats props replace this.
This delegates the logic of extracting the series from the group itself to the parent component of groupChart
| const stats: ReadonlyArray<TimeseriesValue> = statsPeriod | ||
| ? data.filtered | ||
| ? data.filtered.stats?.[statsPeriod] | ||
| : data.stats?.[statsPeriod] | ||
| : EMPTY_STATS; | ||
|
|
||
| const secondaryStats: TimeseriesValue[] | null = | ||
| statsPeriod && data.filtered ? data.stats[statsPeriod] : null; |
There was a problem hiding this comment.
This was previously the logic to extract the stats from the group. As mentioned in the above comment, this is now delegated to the parent to limit the scope of this component.
| @@ -12,6 +12,7 @@ import ShortId from 'sentry/components/shortId'; | |||
| import GroupChart from 'sentry/components/stream/groupChart'; | |||
There was a problem hiding this comment.
I don't believe this component is actually being used anywhere, but out of an abundance of caution, I have still refactored this instance of groupChart
There was a problem hiding this comment.
Hmm it looks like there must have been a recent change that redirects from the discover event details to the issue details page, but I don't know where that redirect is happening or why it was added. Probably why you can't find this anywhere.
| const marklinePoints: number[] = []; | ||
|
|
||
| for (let i = 0; i < stats.length; i++) { | ||
| const point = stats[i]; | ||
| if (point[1] > max) { | ||
| max = point[1]; | ||
| } | ||
| marklinePoints.push(point[1]); | ||
| } |
There was a problem hiding this comment.
Not sure what the purpose of this marklinePoints array was, but it was not being used anywhere.
| const groupStats: ReadonlyArray<TimeseriesValue> = group.filtered | ||
| ? group.filtered.stats?.[statsPeriod] | ||
| : group.stats?.[statsPeriod]; | ||
|
|
||
| const groupSecondaryStats: ReadonlyArray<TimeseriesValue> = group.filtered | ||
| ? group.stats?.[statsPeriod] | ||
| : []; |
There was a problem hiding this comment.
This logic mirrors lines 36-43 in the old groupChart.tsx, and is one example of extracting the stats data in the parent component rather than in the groupChart component
…ping (#70966) This PR refactors the `<GroupChart>` component to have better typing, and to be more readable. It also removes the `group` prop, since the scope of the entire `group` object is beyond what the component needs. Specific details are included the comments. **No design or visual changes should be included in this PR. If you browse the preview and see any visual changes to the events graph, please comment about it and I will rectify.** High level changes: - Updates all instances of `groupChart` to reflect refactor - Replaces `group` and `statsPeriod` props in favor of stats and secondary stats data itself - Removes all uses of `let` in groupChart - Better typing
This PR refactors the
<GroupChart>component to have better typing, and to be more readable. It also removes thegroupprop, since the scope of the entiregroupobject is beyond what the component needs. Specific details are included the comments.No design or visual changes should be included in this PR. If you browse the preview and see any visual changes to the events graph, please comment about it and I will rectify.
High level changes:
groupChartto reflect refactorgroupandstatsPeriodprops in favor of stats and secondary stats data itselfletin groupChart