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(performance): Implement new breadcrumb view in trace drawer #74243

Closed
wants to merge 1 commit into from

Conversation

leeandher
Copy link
Member

What's in this PR

Okay so this allows the new UI to appear in the trace explorer, but with a few caveats. The controls look a bit different than in the drawer and it has a fixed height and inner scroll.

This was sort of a rush job since I start vacation soon and wanted there to at least be an option for the team while I'm out but I'm not super pleased with this user experience, since we still have the inner scroll that the whole redesign pointedly removes.

image

It also creates some code duplication since the buttons used in the drawer are similar, but not the same, since the controls weren't designed with the trace drawer in mind. I tried to reduce the duplication with a wrapping hook, but it's a bit messy.

An alternative

IF we're open to the idea of popping open the drawer experience, we can get away with dropping it in really easily with no code smell:

// static/app/views/performance/newTraceDetails/traceDrawer/details/transaction/index.tsx

 {hasNewTimelineUI ? (
// Provided we refactor to omit `group` prop, or find one from the trace (I didn't look into this)
  <BreadcrumbsDataSection event={event}   />
) : (
  <BreadCrumbs event={event} organization={organization} />
)}

This would make the two components identical, at the cost of covering the rest of the trace drawer with the global drawer when the user prompts it. Not sure how people feel about it, though so I'll leave this PR up for discussion.
image

If we decide the alternative is a better call, please close out this PR.

Feel free to modify this PR to make appropriate changes as we see fit since I'll be away for a bit.

@leeandher leeandher requested a review from a team July 12, 2024 23:20
@leeandher leeandher requested a review from a team as a code owner July 12, 2024 23:20
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 12, 2024
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 36.20690% with 37 lines in your changes missing coverage. Please review.

Project coverage is 78.12%. Comparing base (9576387) to head (33db12d).
Report is 987 commits behind head on master.

Files Patch % Lines
.../details/transaction/sections/traceBreadcrumbs.tsx 0.00% 31 Missing ⚠️
...ts/events/breadcrumbs/breadcrumbsDrawerContent.tsx 75.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #74243      +/-   ##
==========================================
- Coverage   78.13%   78.12%   -0.01%     
==========================================
  Files        6665     6666       +1     
  Lines      297914   297953      +39     
  Branches    51279    51280       +1     
==========================================
+ Hits       232768   232773       +5     
- Misses      58871    58905      +34     
  Partials     6275     6275              
Files Coverage Δ
...ents/events/breadcrumbs/breadcrumbsDataSection.tsx 91.30% <ø> (ø)
...ponents/events/breadcrumbs/breadcrumbsTimeline.tsx 81.48% <100.00%> (+1.48%) ⬆️
...tatic/app/utils/analytics/issueAnalyticsEvents.tsx 100.00% <ø> (ø)
...eDetails/traceDrawer/details/transaction/index.tsx 88.23% <100.00%> (+0.73%) ⬆️
...ts/events/breadcrumbs/breadcrumbsDrawerContent.tsx 87.87% <75.00%> (-2.29%) ⬇️
.../details/transaction/sections/traceBreadcrumbs.tsx 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

@getsantry
Copy link
Contributor

getsantry bot commented Aug 5, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Aug 5, 2024
@getsantry getsantry bot closed this Aug 13, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2024
@Abdkhan14 Abdkhan14 reopened this Oct 1, 2024
@getsantry getsantry bot removed the Stale label Oct 2, 2024
@getsantry getsantry bot added Stale and removed Stale labels Oct 24, 2024
@getsantry getsantry bot added Stale and removed Stale labels Nov 15, 2024
@getsantry getsantry bot added Stale and removed Stale labels Dec 8, 2024
@getsantry getsantry bot added the Stale label Dec 31, 2024
@getsantry getsantry bot closed this Jan 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants