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

style: implement new flyout for Insights exemplar spans #81273

Closed
bcoe opened this issue Nov 25, 2024 · 2 comments · Fixed by #82534 or #83301
Closed

style: implement new flyout for Insights exemplar spans #81273

bcoe opened this issue Nov 25, 2024 · 2 comments · Fixed by #82534 or #83301

Comments

@bcoe
Copy link
Member

bcoe commented Nov 25, 2024

Problem Statement

The insights flyout should be updated to match issues:

Image

Product Area

Insights

@getsantry
Copy link
Contributor

getsantry bot commented Nov 25, 2024

Routing to @getsentry/product-owners-insights for triage ⏲️

@gggritso
Copy link
Member

gggritso commented Jan 7, 2025

Reopening since I reverted the PR.

@gggritso gggritso reopened this Jan 7, 2025
gggritso added a commit that referenced this issue Jan 13, 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.
andrewshie-sentry pushed a commit that referenced this issue Jan 22, 2025
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

## 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 pulled up into
the parent
- 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.
andrewshie-sentry pushed a commit that referenced this issue 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.