Skip to content

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 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

3 participants