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(insights): Use GlobalDrawer instead of details panel II #83301

Merged
merged 38 commits into from
Jan 13, 2025

Conversation

gggritso
Copy link
Member

Attempt #2. The previous PR had to be reverted because I didn't account for URL behaviour.

This PR is very similar, but now opening and closing the span samples drawer is done in a special hook that watches for the required URL parameters.


Closes #81273

Replaces all Insights side panels with new GlobalDrawer implementations that match the drawers in Issues. The visual changes are minimal

  1. Slightly different animations
  2. A "Close" button on the top left of the drawer
  3. Scroll bars on the left of the drawer
  4. Tighter padding
  5. Panel doesn't re-animate as often on clicking another transaction

There are small feature changes!

  • no more docking bottom or right. Docking is to the right only
  • panels now have closing animations

Before:
Screenshot 2024-12-23 at 11 42 33 AM
Screenshot 2024-12-23 at 11 42 44 AM
Screenshot 2024-12-23 at 11 43 07 AM

After:
Screenshot 2024-12-23 at 11 43 19 AM
Screenshot 2024-12-23 at 11 43 55 AM
Screenshot 2024-12-23 at 11 44 44 AM
Screenshot 2024-12-23 at 12 04 50 PM

Code Changes

GlobalDrawer is invoked with a hook, rather than rendered inline. So, I had to make some changes.

Before:

  • rendering panels mostly unconditionally like <CacheSamplesPanel />
  • each sample panel checks the URL to see if it's supposed to be open
  • if open, renders itself, otherwise stays invisible
  • on close, updates the URL, which re-renders and hides

This is problematic for a bunch of reasons:

  1. When components can decide to not render themselves, the code flow is confusing. Parents should generally decide when to render children
  2. When a component is rendered but hidden, it runs its side effects. This means that in our case, network requests were sometimes going out, even though the panel isn't visible

After:

  • the panel is rendered using a useEffect, the effect detects a necessary URL parameter or state, and open the panel if necessary. All conditions that the panel used to check for itself are passed as parameters to the hooks
  • the panel is rendered straight, it doesn't have any conditionals to check if it's supposed to be open
  • on close, the panel hides itself, and tells the parent to update the URL or state as necessary

This is tidier, avoids component side-effects, and preserves closing animations! Plus, this means I can re-use a header component, make the analytics tracking consistent, etc.


This only leaves a small handful of users of DetailPanel, mostly in the trace view.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 13, 2025
@gggritso gggritso marked this pull request as ready for review January 13, 2025 15:28
@gggritso gggritso requested a review from a team as a code owner January 13, 2025 15:28
@gggritso gggritso merged commit 48826bf into master Jan 13, 2025
44 checks passed
@gggritso gggritso deleted the feat/insights/new-slideout-panel-design-ii branch January 13, 2025 20:14
andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025
Attempt #2. The [previous
PR](#82534) had to be reverted
because I didn't account for URL behaviour.

This PR is very similar, but now opening and closing the span samples
drawer is done in a special hook that watches for the required URL
parameters.

---

Closes #81273

Replaces all Insights side panels with new `GlobalDrawer`
implementations that match the drawers in Issues. The visual changes are
minimal

1. Slightly different animations
2. A "Close" button on the top left of the drawer
3. Scroll bars on the left of the drawer
4. Tighter padding
5. Panel doesn't re-animate as often on clicking another transaction

There are small feature changes!

- no more docking bottom or right. Docking is to the right only
- panels now have closing animations

**Before:**
<img width="829" alt="Screenshot 2024-12-23 at 11 42 33 AM"
src="https://github.com/user-attachments/assets/78e3631b-7694-42eb-b3f0-149f92307748"
/>
<img width="817" alt="Screenshot 2024-12-23 at 11 42 44 AM"
src="https://github.com/user-attachments/assets/f24b2a0e-3adb-414d-b965-eeb5715f7a9e"
/>
<img width="838" alt="Screenshot 2024-12-23 at 11 43 07 AM"
src="https://github.com/user-attachments/assets/9753b6da-a6fb-464b-bc3e-25f5126fa4e5"
/>

**After:**
<img width="767" alt="Screenshot 2024-12-23 at 11 43 19 AM"
src="https://github.com/user-attachments/assets/b500af97-7493-4f2f-9330-6190e31eaac1"
/>
<img width="774" alt="Screenshot 2024-12-23 at 11 43 55 AM"
src="https://github.com/user-attachments/assets/332569ad-b715-4365-b000-188b64747944"
/>
<img width="563" alt="Screenshot 2024-12-23 at 11 44 44 AM"
src="https://github.com/user-attachments/assets/338ec721-471e-4760-b028-6df850df0e8e"
/>
<img width="483" alt="Screenshot 2024-12-23 at 12 04 50 PM"
src="https://github.com/user-attachments/assets/db20d3a8-9958-4993-8ea0-6781e1467a80"
/>

## Code Changes

`GlobalDrawer` is invoked with a hook, rather than rendered inline. So,
I had to make some changes.

**Before:**

- rendering panels mostly unconditionally like `<CacheSamplesPanel />`
- each sample panel checks the URL to see if it's supposed to be open
- if open, renders itself, otherwise stays invisible
- on close, updates the URL, which re-renders and hides

This is problematic for a bunch of reasons:

1. When components can decide to not render themselves, the code flow is
confusing. Parents should generally decide when to render children
2. When a component is rendered but hidden, it runs its side effects.
This means that in our case, network requests were sometimes going out,
even though the panel isn't visible

**After:**

- the panel is rendered using a `useEffect`, the effect detects a
necessary URL parameter or state, and open the panel if necessary. All
conditions that the panel used to check for itself are passed as
parameters to the hooks
- the panel is rendered straight, it doesn't have any conditionals to
check if it's supposed to be open
- on close, the panel hides itself, and tells the parent to update the
URL or state as necessary

This is tidier, avoids component side-effects, and preserves closing
animations! Plus, this means I can re-use a header component, make the
analytics tracking consistent, etc.

---

This only leaves a small handful of users of `DetailPanel`, mostly in
the trace view.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

style: implement new flyout for Insights exemplar spans
2 participants