Closed Bug 1256422 Opened 8 years ago Closed 8 years ago

Move the breadcrumbs into a bottom toolbar

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [btpp-fix-later])

Attachments

(3 files)

      No description provided.
Attached patch 1256422.patchSplinter Review
Attachment #8730352 - Flags: review?(bgrinstead)
There has been some discussions to remove it altogether. CC'ing Helen.
Whiteboard: [btpp-fix-later]
Comment on attachment 8730352 [details] [diff] [review]
1256422.patch

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

::: devtools/client/themes/inspector.css
@@ +13,5 @@
>  }
>  #inspector-searchlabel {
>    overflow: hidden;
>  }
> +#inspector-breadcrumbs-toolbar {

My 2 cents: it looks better if we override the background color on inspector-breadcrumbs-toolbar to be the normal body background color.  Especially on the dark theme.  What do you think Helen?  You could apply the patches locally or possibly check out the screenshots on these try pushes if they work out:

* Current patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb0e6f05cbd1
* With proposed change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e5052318720

@@ +16,5 @@
>  }
> +#inspector-breadcrumbs-toolbar {
> +  padding: 0px;
> +  border-bottom-width: 0px;
> +  border-top-width: 1px;

I wish this didn't need the border (with my suggested change above applied), but it would look funny if the crumbs are just floating around.  Probably that would be possible if we made the crumbs absolutely positioned at the bottom of the viewport, but not needed for this bug if we want to land something first without a big redesign of the crumbs feature (which I think we do)
Attachment #8730352 - Flags: ui-review?(hholmes)
Attachment #8730352 - Flags: review?(bgrinstead)
Attachment #8730352 - Flags: review+
Alternate patch with the idea from comment 4 in case you want to apply them locally
Attachment #8730352 - Flags: ui-review?(hholmes) → ui-review+
https://hg.mozilla.org/mozilla-central/rev/82a7735069d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1260053
Depends on: 1260145
Depends on: 1262668
Attached image breadcrumbs-spec.png
So I messed around with the breadcrumbs after reading through Bug 1262668, and I agree with both of Brian's points in Comment 4—it'd be nice to keep the background-color the background-color of the Inspector, and I think the border shouldn't be the same as the splitter—it causes some confusion.

So, I'd recommend the background be var(--theme-body-background), and that we introduce a new variable for #fafafa (border for non-splitters).

In my mockup I'm also using different colors than we currently have in the breadcrumbs and inspector, but those I think should be addressed in bug 1246313.
I've updated: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#HTML_breadcrumbs

I've also updated the "UI tour" page, so it's consistent: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/UI_Tour.

Marking as dev-doc-complete, but let me know if I need anything else, please.
See Also: → 1272031
Note that people are complaining on Stack Overflow about the breadcrumb bar being moved to the bottom.
Unfortunately the bug doesn't give any reason why this was done. At the moment it looks like we just lost vertical space by this change.

Gabriel, could you please explain it?

Sebastian
Flags: needinfo?(gl)
(In reply to Sebastian Zartner [:sebo] from comment #11)
> Note that people are complaining on Stack Overflow about the breadcrumb bar
> being moved to the bottom.
> Unfortunately the bug doesn't give any reason why this was done. At the
> moment it looks like we just lost vertical space by this change.
> 
> Gabriel, could you please explain it?
> 
> Sebastian

There are a couple of reasons here that made us move it to the bottom. We wanted to repurpose the space that the breadcrumbs occupied - to make new and existing features more visible. The start of this was adding new buttons to that space - the create new node in this case. There were additional work to add some of the toolbox buttons to that space as well. There were discussions to remove the breadcrumbs altogether, but we ultimately felt we should move it to the bottom to make it less obtrusive. We will continue to enhance that newly freed top space in the inspector in Q4.
Flags: needinfo?(gl)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.