-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unfortunately the re-use is not as consistent as I hoped.
This reverts commit 3e54ef6.
This is what the tests expect, and it makes sense
It's optional now
github-actions
bot
added
the
Scope: Frontend
Automatically applied to PRs that change frontend components
label
Jan 13, 2025
edwardgou-sentry
approved these changes
Jan 13, 2025
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 minimalThere are small feature changes!
Before:
After:
Code Changes
GlobalDrawer
is invoked with a hook, rather than rendered inline. So, I had to make some changes.Before:
<CacheSamplesPanel />
This is problematic for a bunch of reasons:
After:
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 hooksThis 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.