Skip to content

Add an [X] button on track names in the timeline to quickly hide unin…#4170

Merged
canova merged 2 commits into
firefox-devtools:mainfrom
fqueze:hide-track
Aug 9, 2022
Merged

Add an [X] button on track names in the timeline to quickly hide unin…#4170
canova merged 2 commits into
firefox-devtools:mainfrom
fqueze:hide-track

Conversation

@fqueze

@fqueze fqueze commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

…teresting tracks or processes.
Capture d’écran 2022-08-03 à 20 15 33
Capture d’écran 2022-08-03 à 20 15 44

deploy preview

@fqueze fqueze requested a review from canova August 3, 2022 18:16
@fqueze fqueze requested a review from a team as a code owner August 3, 2022 18:16
@codecov

codecov Bot commented Aug 3, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4170 (05d9265) into main (612ee29) will decrease coverage by 0.02%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main    #4170      +/-   ##
==========================================
- Coverage   88.48%   88.45%   -0.03%     
==========================================
  Files         280      280              
  Lines       24518    24528      +10     
  Branches     6535     6535              
==========================================
+ Hits        21695    21697       +2     
- Misses       2621     2629       +8     
  Partials      202      202              
Impacted Files Coverage Δ
src/components/timeline/GlobalTrack.js 68.42% <20.00%> (-2.23%) ⬇️
src/components/timeline/LocalTrack.js 70.11% <20.00%> (-3.06%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mstange

mstange commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

UI feedback:

  • I'd like to see a version where the button is at the start of the track, rather than between the track title and the timeline.
  • As for the icon, I think it would be better to make it look similar to the tab close button in Firefox: Display the x at all times, and show a rounded square around it when the button is hovered

@mstange mstange self-requested a review August 3, 2022 18:29
@fqueze

fqueze commented Aug 3, 2022

Copy link
Copy Markdown
Contributor Author

UI feedback:

  • I'd like to see a version where the button is at the start of the track, rather than between the track title and the timeline.

Why?

The end seemed the logical place to me, and matches what we see in Firefox tabs.
Also, when interacting with it, I find it quite useful to have all the icons line up, so that when you hide something (be it a local track or a process), the next track appears at the same position and you can continue clicking without moving the mouse until you are done hiding all the tracks you don't need. Given the local tracks are indented, I don't think placing the button at the start would make it possible to have them line up.

  • As for the icon, I think it would be better to make it look similar to the tab close button in Firefox: Display the x at all times, and show a rounded square around it when the button is hovered

I agree about the rounded square background when the button is hovered, that would make the size of the click target visible.

The Firefox behavior when the tab bar is crowded is to show the close icon only for the selected tab. (I think it used to show it on other tabs on hover, but I don't see this behavior when testing a current version.)

The reason why I made the icon appear only on hover is to avoid wasting space in an area where we are already tight on space (some thread names aren't fully visible, and that's annoying, especially when the end of the name contains something that's required to identify the thread, Eg "BackgroundThreadPool #1").

I'm not sure which problem you want to solve when you say "Display the x at all times". Are you annoyed by the motion when moving the mouse if something appears on hover, or are you concerned about low discoverability?

One thing we could do is to make the X be always visible on the selected track, and add a rounded square background when the icon is hovered. That would match the Firefox behavior pretty well.

I think being able to hide a non-selected track with a single click (when the icon appears on hover) is useful, but I could live with needing to select the track first if the on-hover behavior has downsides I haven't noticed.

@fqueze

fqueze commented Aug 3, 2022

Copy link
Copy Markdown
Contributor Author

As for the icon, I think it would be better to make it look similar to the tab close button in Firefox: Display the x at all times, and show a rounded square around it when the button is hovered

I agree about the rounded square background when the button is hovered, that would make the size of the click target visible.

Actually, UI consistency also matters, and I'm not sure introducing another X icon in addition to the one we already have would really be an improvement.

@mstange

mstange commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

Here's what I had in mind:

Screen.Recording.2022-08-03.at.4.09.33.PM.mov

@mstange

mstange commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

The true answer to your questions about "why?" is: "not sure, this is very intuition-based, and screenshots don't convey the full experience anyway, you have to actually play with it and see how it feels".

The most important aspect to me is the icon - the circle icon is based on the searchfield clear button of an old version of macOS. As for consistency, I'd be happy to change the searchfield so that we use the same icon in both places. By the way, the legal footer is already using a different x.

And I've already gone back on the idea to keep the x visible at all times; it looked very crowded. Making it visible only on track hover adds more flashing as you move the mouse and more instability, but maybe that's the lesser evil.

As for the position, it turns out I don't like either option very much :)
Vertically it sits in the middle, with awkward dead area above and below.

Horizontally I think I have a slight preference for having it on the left, because it'll be in a fixed-width "pocket" of space. When putting it on the right, the space to the left of the icon varies based on the label string length, so the icon's position has a bit more of an arbitrarily floating feeling.

@mstange

mstange commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

I think position-wise, the most important question is: Which position is the best at avoiding accidental clicks?

@gregtatum

Copy link
Copy Markdown
Member

Horizontally I think I have a slight preference for having it on the left, because it'll be in a fixed-width "pocket" of space. When putting it on the right, the space to the left of the icon varies based on the label string length, so the icon's position has a bit more of an arbitrarily floating feeling.

This would be my feedback, having a clear space where text doesn't get resized seems better.

Which position is the best at avoiding accidental clicks?

The left would be less likely to be clicked I would think.

I don't really have an opinion on the stylings of the X.

Making it visible only on track hover adds more flashing as you move the mouse and more instability

An opacity transition of 100ms-200ms may help a bit here.

@mstange mstange removed their request for review August 8, 2022 20:42
@mstange

mstange commented Aug 8, 2022

Copy link
Copy Markdown
Contributor

After thinking about it more, I agree with Florian now that the PR is good enough in the current state to land. Once it lands, I'll make a PR on top of it which switches out the icon and adds mouseover + mousedown feedback.

The code looks fine to me but I'll let @canova do the actual review.

@canova canova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, it looks good to me!

Also thanks for all the feedback! I trust you 100% since I'm really not great at UX 😄 Markus' conclusion sounds good to me.

Comment on lines +288 to +298
<Localized
id="TrackNameButton--hide-process"
attrs={{ title: true }}
>
<button
type="button"
className="timelineTrackCloseButton"
title="Hide process"
onClick={this._hideCurrentTrack}
/>
</Localized>

@canova canova Aug 9, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking if we should hide this button when there is only one hideable track left. hideGlobalTrack action already handles this case and doesn't remove the last track but it could be good to also hide this button for this case so people don't see this option at all.
But if we decide to do it, we can still do it as a follow-up. I'm okay with landing it as is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea! We should also apply it to the context menu item, which currently is not disabled in that case, but does nothing.
Capture d’écran 2022-08-10 à 11 44 30

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should also fix this one.

@canova canova merged commit d720364 into firefox-devtools:main Aug 9, 2022
@canova canova mentioned this pull request Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants