Closed Bug 1396349 Opened 2 years ago Closed 2 years ago

Remove the collapse sidebar pane button in the Inspector

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional, firefox58 verified)

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- fix-optional
firefox58 --- verified

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #8903996 - Flags: review?(pbrosset)
Attachment #8903995 - Attachment is obsolete: true
Attachment #8903995 - Flags: review?(pbrosset)
Why are we removing the collapse button? Can you point to mockups and reasons that show and explain the change?
Other sidebars in the toolbox are collapsible (netmonitor, storage, debugger), are we going to make them non-collapsible too?
Flags: needinfo?(gl)
This is under inspector light in https://docs.google.com/document/d/1AJ7oHC7akXJiliheHjhFc4fTonaN31Eo_P2M8gssL8s/edit#heading=h.36hd86a2u1dp. We are actually removing that entire toolbar and making the search bar expandable like the debugger. The reasoning for this is a 2 parter:
(1) We already have a conceptual model from the debugger and code editors that search prompts appear from cmd/ctrl + F.
(2) Regain some of the vertical height from things getting bigger.

So, the next question will probably be what happens to the Add New Node and Color picker button. The Color picker seems to be moving to the bottom and going with the breadcrumbs. The mockup doesn't actually keep the add new node button anywhere, but presumably it can go next to the color picker as well. 

The mockup also shows removal of the collapse button of the debugger. I think there is a plan to adopt the way Atom collapses sidebars.

For now, it seems removing the collapse button seems relatively safe for the inspector since we rarely would want to hide the sidebar and Chrome doesn't do it.
Flags: needinfo?(gl)
(In reply to Gabriel [:gl] (ΦωΦ) from comment #4)
> This is under inspector light in
> https://docs.google.com/document/d/
> 1AJ7oHC7akXJiliheHjhFc4fTonaN31Eo_P2M8gssL8s/edit#heading=h.36hd86a2u1dp. We
> are actually removing that entire toolbar and making the search bar
> expandable like the debugger. The reasoning for this is a 2 parter:
> (1) We already have a conceptual model from the debugger and code editors
> that search prompts appear from cmd/ctrl + F.
> (2) Regain some of the vertical height from things getting bigger.
Regaining vertical height sounds good, and having the same search experience across tools too (although the debugger has a clickable area to trigger the search tool too).
I guess I'm just worried about removing a feature. We've done this in the past and it's always been hard.

> So, the next question will probably be what happens to the Add New Node and
> Color picker button. The Color picker seems to be moving to the bottom and
> going with the breadcrumbs. The mockup doesn't actually keep the add new
> node button anywhere, but presumably it can go next to the color picker as
> well. 
Sure. That would work. In any case, we should keep the add node button. We have one in the rule-view too. And it's an important feature when you think of the designer workflow idea.

> The mockup also shows removal of the collapse button of the debugger. I
> think there is a plan to adopt the way Atom collapses sidebars.
Oh ok. I was not aware with this so I downloaded Atom. I see what you mean now.
Are we adding this at the same time as we're removing the collapse buttons? I suggest we do. To avoid removing a feature and then later re-introducing it differently.

> For now, it seems removing the collapse button seems relatively safe for the
> inspector since we rarely would want to hide the sidebar and Chrome doesn't
> do it.
Hmm, I don't know if we can make such a call. How do you know we rarely want to hide the sidebar?

I really like all the Photon style refresh Victoria and you have been doing in 57. I would advise the following: let's land as much as can in terms of color and tab changes. 57's cycle is almost over, so I think it's safe to land those 2 changes because they're already in nightly and really safe.
But I would advise against landing feature changes in 57. If we just wait a couple weeks, then we can land in 58 both the removal of collapse icons and the new Atom-like collapsers, across all tools. Rather than just remove the inspector icon, which will seem arbitrary to our users.
I think this meets my expectation for landing after 57 as well.
Comment on attachment 8903996 [details] [diff] [review]
1396349.patch [1.0]

Review of attachment 8903996 [details] [diff] [review]:
-----------------------------------------------------------------

Simple code removal. No problem for me to R+ apart from the conversation we just had related to landing in 58 along with the new sidebar collapse UX.
Attachment #8903996 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f52baae3f9d
Remove the collapse sidebar pane button in the Inspector. r=pbro
https://hg.mozilla.org/mozilla-central/rev/6f52baae3f9d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I have reproduced this bug with Nightly 57.0a1 (2017-09-03) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly 58.0a1.

Build ID 	20170926100259
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
QA Whiteboard: [bugday-20170927]
I have successfully reproduced this bug with Nightly 57.0a1 (2017-09-03)  on windows 10 (32-bit)

this bug is verified fix with  latest nightly 58.0a1 (2017-09-26) (32-bit)

Build ID: 20170926100259
Mozilla/5.0 (Windows NT 10.0; rv:58.0) Gecko/20100101 Firefox/58.0

[bugday-20170927]
As per Comment 10 and Comment 11, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
I just realized that we didn't have time to do what we said in comment 5: work on the new collapser mechanism consistently across the tools.
So, basically, we just removed the collapse functionality from the inspector in 58, without taking advantage of the space now available, and without giving a new way to collapse the sidebar.

So, the redesign isn't ready yet, and it will look like to users of 58 that we just introduced a regression.
I suggest rolling this back quickly and working on the whole toolbar redesign at a later stage.

Gabriel: can you please confirm I'm not missing anything?
Flags: needinfo?(gl)
I am actually comfortable with keeping this removed for 58. Here are a couple of reasons why:

In Bug 1369945, we actually reuse the same collapser to toggle between the 3-pane inspector. I would be comfortable with users forgetting that this behaviour existed and its association. So, we can reintroduce it with the 3 pane inspector toggle.

Since the collapser is rendered with React, you can actually see it move shift the eyedropper to the left when the toolbox is opened. This is a hit in terms of perceived performance when we were working on Photon.

There is actually very little reason to want to collapse the sidebar in the inspector imo. Chrome does not offer this behaviour either.
Flags: needinfo?(gl)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.