-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 detail panels
#82534
Open
gggritso
wants to merge
28
commits into
master
Choose a base branch
from
feat/insights/new-slideout-panel-design
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
Dec 23, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #82534 +/- ##
===========================================
- Coverage 86.70% 80.44% -6.27%
===========================================
Files 9414 7316 -2098
Lines 538327 322560 -215767
Branches 21047 21036 -11
===========================================
- Hits 466745 259472 -207273
+ Misses 71175 62681 -8494
Partials 407 407 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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 pulled up into the parentThis 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.