Profiler UI/UX tweaks and bug-fixes #1112
Conversation
1. Replace interaction commit circles with squares 2. Color commits in both the main and right-side pane based on their duration (to tie into other chart views) 3. Remove click-to-select commit because this surprised Sophie and Shirley initially
bvaughn
changed the title
|
Okedoke. This PR is ready for review. It fixes #1104 (so we should probably do another browser release after it's merged) and wraps up the remaining low-hanging-fruit from the umbrella issue. I've split the remaining larger umbrella items off into separate issues tagged with a new plugin: profiler label. |
|
Do you mind updating the preview page so I could try it without building? |
|
Just pushed an update. |
|
A few nits (might not be related to your changes, just general observations):
|
|
In general I find this interaction disorienting. I get teleported to another screen and don't know how to go back. I know how to go back now (just click on preselected interaction) but it's still confusing me sometimes. I think I might be expecting a "back" button or some visual cue to tell me I can go back to previous list easily. |
|
Overall I'm not sure I dig black fill as the selected commit. I understand you wanted to make it more prominent, but it confuses me that we use color both to indicate the cost and to indicate what's selected, and selecting "hides" the cost hint from us. Have you explored other options, e.g. a prominent black border while keeping the background as is? |
Yes. I probably haven't explored them all, so maybe there's a good one I've not thought of. In general, theming makes this tricky– since it limits our color palette. 😦 The above is some good feedback, but most of it seems unrelated to this PR (except for the added black box to indicate the currently selected commit in the interactions panel) and it feels a bit hard to respond to since it's just a bullet list of comments. Do you mind filing them as individual issues with the profiler tag?
I also don't see what you're referring to with this one. |
Zoom in on the image. You’ll see there’s a strange 1px white strip between black and grey. It looks odd like a glitch. |
|
I edited your original comment and added some inline responses. (I don't know of a better way to respond to a huge multi-bullet comment like that.) I'm going to push a couple tweaks for the very small items, but I think most things should be filed as follow up issues since this is a confusing format :D |
|
Pushed a few tweaks to low hanging fruit! |
|
Sorry, I wasn’t tracking individual issues and which changes were associated with which PR so it’s more of a braindump from somebody trying this for the first time. I can later come back and file a single issue for each of these — it just felt excessive if most are small changes or not even necessarily fixable. |
|
Totally makes sense 😄 I responded inline with some comments and tagged a few as already completed. |
I’m not scrutinizing just to nitpick :-) We didn’t have a tabbed UI before so I didn’t often click on different parts of the toolbar. Now that we have a tabbed interface, I click on it quite a bit and find myself accidentally selecting things on it. Seems like an easy fix so I flagged it. I can refrain from flagging small changes like this, but I selected the toolbar content two times by accident during a five minute testing period. And in general I think we shouldn’t avoid polish because some older parts aren’t polished. |
|
That was more of a disclaimer that "I don't think we've thought about this level of nuance in other part of DevTools" – I already made the change in this particular place 😄 Thanks for pointing it out. |
We could automatically select the first non-empty commit. We could also tweak the empty state text to suggest selecting a commit from the upper right corner or using arrow keys. |
Maybe... I think this might be a bit disorienting though.
I like this! |
|
Actually, thinking more about that empty state– I want to look closer into this. I believe I read it too fast and first and misunderstood what the screenshot was showing. I assume this is the suspense demo, the "Debugger" root? |
|
Ah, I believe this is a very special case, since the "Debugger" is mostly just rendering "null" unless you type "/" in the demo app. In most cases, even 0 duration commits should still show an all-gray-tree. I will tweak this messaging a bit though to hopefully make this clearer. I think this message makes sense for the "ranked" view (which could be empty if performance.now timing returned 0) but not the flame graph view. |
I’m talking about 1px white strip.
|
I was using it on UFI in Workplace. When you say “couldn’t reproduce” what do you mean precisely? Do you mean that pressing left and right arrows to expand and collapse a single tag with many children always brings its start into view? |
The circle in my GIF shows when the mouse is down. You can see that I release it outside the pane. When I enter the pane again, it still thinks the mouse is down (and scrolls through commits) even though I released it. The only way to stop it from thinking I’m holding he mouse down is to hold it down again and release. This sounds like something that should be possible to solve. I just don’t remember how. |
Oh! 😆 I see it now! Thanks for circling it.
Yes, using the "deeply nested" component in the example test app, I couldn't repro it. Seemed like I should have been able to.
Oh! The gif wasn't animating for me earlier so I wasn't seeing that. Now I do. Hm... I listen for mouse-up on the window, but if you mouse out of the window entirely...hm. File a follow up issue for it. I don't know the fix off the top of my head. |
|
I just pushed a commit (and deployed to the preview URL) that improves the UI/UX for commits with a duration of 0. Try again? 😄 |
|
I created a few spin off issues for remaining feedback that I didn't address already. Feel free to create more for anything I missed. 😄 |
|
Anything blocking this from merge, @gaearon ? |
Resolves #1104 and umbrella issue #1099
Interactions
Elements panel
General