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.

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

feat(ui): Add release bubbles to issue details #86950

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Mar 12, 2025

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 12, 2025
@billyvg billyvg changed the title ref(ui): Change useReleaseMarkLineSeries to use useReleaseStats hook feat(ui): Add release bubbles to issue details Mar 12, 2025
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #86950   +/-   ##
=======================================
  Coverage   87.73%   87.73%           
=======================================
  Files        9892     9892           
  Lines      561300   561220   -80     
  Branches    22129    22110   -19     
=======================================
- Hits       492475   492408   -67     
+ Misses      68423    68410   -13     
  Partials      402      402           

Comment on lines 198 to 209
const releaseSeries = useReleaseMarkLineSeries({
group,
releases: hasReleaseBubblesSeries ? [] : releases,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to pass in releases and only do so if bubbles is turned off

@billyvg billyvg force-pushed the feat-ui-issue-details-release-mark-lines-use-release-stats branch from 05b175f to fb1d633 Compare March 14, 2025 18:14
@billyvg billyvg force-pushed the feat-ui-issue-details-release-mark-lines-use-release-stats branch from fb1d633 to a321c2d Compare March 18, 2025 14:52
@billyvg billyvg force-pushed the feat-ui-issue-details-release-mark-lines-use-release-stats branch from a321c2d to 3e08f31 Compare March 18, 2025 15:29
@billyvg billyvg force-pushed the feat-ui-issue-details-release-mark-lines-use-release-stats branch from 3e08f31 to 91d201a Compare March 18, 2025 19:06
@billyvg billyvg force-pushed the feat-ui-issue-details-release-mark-lines-use-release-stats branch from 91d201a to 5ce604e Compare March 18, 2025 19:32
)}
showReleaseAs="line"
/>
<div style={{height: '220px'}}>
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to remove the height from the drawer to support the issue details chart which uses the default 100px height.

},
},
{
staleTime: 0,
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
staleTime: 0,
staleTime: Infinity,

Comment on lines +233 to +236
const releaseSeries = useReleaseMarkLineSeries({
group,
releases: hasReleaseBubblesSeries && showReleasesAs !== 'line' ? [] : releases,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the useReleaseStats hook up into <EventGraph> so that 1) releases data is shared between the two release series (line/bubble) and 2) so that we can better control which display type to show (alternatively I could have introduced an enabled option to both hooks?).

Comment on lines +251 to +253
showSummary={false}
showReleasesAs="line"
disableZoomNavigation
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the chart that gets rendered in the drawer, these are the custom options that differentiate it from the main chart.

Comment on lines +513 to +518
{...(disableZoomNavigation
? {
isGroupedByDate: true,
dataZoom: chartZoomProps.dataZoom,
}
: chartZoomProps)}
Copy link
Member Author

Choose a reason for hiding this comment

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

For the chart inside of the drawer -- we dispatch a "zoom" action to it so that it sets the proper time boundaries based on the release bucket timestamps.

Comment on lines +74 to +84
e.getEchartsInstance().dispatchAction({
type: 'dataZoom',
batch: [
{
// data value at starting location
startValue: startTs,
// data value at ending location
endValue: endTs,
},
],
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Sooo in TimeSeriesWidgetsViz we trim the series that the chart renders to be within the release bucket timeframe, but this could be an alternative where we have the chart just "zoom" into the proper timeframe.

@billyvg billyvg marked this pull request as ready for review March 18, 2025 19:52
@billyvg billyvg requested review from a team as code owners March 18, 2025 19:52
@billyvg billyvg requested review from a team and removed request for a team March 18, 2025 19:52
@billyvg billyvg marked this pull request as draft March 18, 2025 20:27
@billyvg
Copy link
Member Author

billyvg commented Mar 18, 2025

Putting this back in draft, going to do some design reviews with Vu + Vasudha first.

@billyvg billyvg force-pushed the feat-ui-issue-details-release-mark-lines-use-release-stats branch from a50a741 to 13663aa Compare March 20, 2025 16:11
@billyvg billyvg force-pushed the feat-ui-issue-details-release-mark-lines-use-release-stats branch from 13663aa to 4d055f7 Compare March 21, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant