feat(events-graph): Add storybook for new events graph component #70866
Conversation
github-actions
bot
added
the
Scope: Frontend
| // [1715554800, 126], | ||
| [1715558400, 112], | ||
| // [1715562000, 126], | ||
| [1715565600, 113], | ||
| // [1715569200, 118], | ||
| [1715572800, 87], | ||
| // [1715576400, 88], | ||
| [1715580000, 59], | ||
| // [1715583600, 54], | ||
| [1715587200, 55], | ||
| // [1715590800, 52], | ||
| [1715594400, 48], | ||
| // [1715598000, 62], | ||
| [1715601600, 86], | ||
| // [1715605200, 100], | ||
| [1715608800, 121], | ||
| // [1715612400, 124], | ||
| [1715616000, 129], | ||
| // [1715619600, 149], | ||
| [1715623200, 141], | ||
| // [1715626800, 132], | ||
| [1715630400, 133], | ||
| // [1715634000, 127], | ||
| [1715637600, 82], |
There was a problem hiding this comment.
Half of the 24 entries in this series are commented out to align the storybook more with the existing mockups, which currently only 12 bars. This is what it looks like with all 24 entries enabled:
| showSecondaryPoints?: boolean; | ||
| }; | ||
|
|
||
| function GroupStatusChart({ |
There was a problem hiding this comment.
This new component is largely the same as the pre-existing <GroupChart/> component, but I would still prefer to have work contained in this new file to avoid any unintended changes to the existing component.
| const EMPTY_STATS: ReadonlyArray<TimeseriesValue> = []; | ||
|
|
||
| type Props = { | ||
| data: Group; |
There was a problem hiding this comment.
It looks like you only need a couple properties on Group, so it would probably be better to just accept stats and filteredStats. That way mocking is easier and consumers of this component don't need access to the whole group.
There was a problem hiding this comment.
I completely agree and would take it another step further - the chart component shouldn't even have the responsibility of deciding whether or not to use the filtered or unfiltered stats, especially since it only uses one. The parent component should just decide which one to use and pass the correct stats down to this component.
|
@malwilley @scttcper thank you for the feedback!! This is infinitely more readable now I'll also put up another PR making the same changes to the existing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #70866 +/- ##
===========================================
- Coverage 91.36% 80.00% -11.37%
===========================================
Files 2861 6506 +3645
Lines 178780 290857 +112077
Branches 31901 50133 +18232
===========================================
+ Hits 163344 232691 +69347
- Misses 15435 57732 +42297
- Partials 1 434 +433
|
|
^ This commit will also round the corners of each bar in the bar chart in prod: before: after: |
) Related ticket: #69691 This PR creates the storybook and initial design for the new events stats graph component, `<GroupStatusChart/>`. **This PR does not change the existing events graph in the issue stream**. The only publicly used file that was modified is miniBarChart.tsx. The `<MiniBarChart/>` component is the underlying component of the existing events graph component, `<GroupChart/>`, and it needs to expose more parameters of the graph as props to allow for the modifications in the new `<GroupStatusChart/>` component to avoid creating an entirely new underlying component\. Prop defaults were set for all new props introduced to ensure that the existing event stats chart is visually unchanged. The new component in its current form:  Remaining work to be done (probably in future PRs): - [ ] Add test file - [ ] Add loading state - [ ] **Swap old events graph with this one** Open design questions: - [ ] ~~What should the loading state look like?~~ Gray box (with rounded edges)
Related ticket: #69691
This PR creates the storybook and initial design for the new events stats graph component,
<GroupStatusChart/>. This PR does not change the existing events graph in the issue stream.The only publicly used file that was modified is miniBarChart.tsx. The
<MiniBarChart/>component is the underlying component of the existing events graph component,<GroupChart/>, and it needs to expose more parameters of the graph as props to allow for the modifications in the new<GroupStatusChart/>component to avoid creating an entirely new underlying component. Prop defaults were set for all new props introduced to ensure that the existing event stats chart is visually unchanged.The new component in its current form:
Remaining work to be done (probably in future PRs):
Open design questions:
What should the loading state look like?Gray box (with rounded edges)