Closed Bug 1428442 Opened 4 years ago Closed 4 years ago

Enable highlighting the tab of more than one tool at a time (like when the debugger is paused)

Categories

(DevTools :: Framework, enhancement, P2)

57 Branch
enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now it is only possible to highlight 1 tool at a time (debugger OR network monitor), it is possible that more than one tool is currently active/has an affect on other tools so this functionality needs to be expanded to account for that.
Do you mean being able to see more than 1 tool at the same time? Like the split console for instance?
If so, then this is a duplicate of bug 1121414.
Flags: needinfo?(yzenevich)
Ah, I should've been more descriptive. This is just related to the indication of an active tool - e.g. the green colored tab highlighting (debugger for example).
Flags: needinfo?(yzenevich)
Oh I see. Thanks for clarifying.
Summary: Enable highlighting more than one tool at a time. → Enable highlighting the tab of more than one tool at a time (like when the debugger is paused)
Severity: normal → enhancement
Priority: -- → P2
Attached patch 1428442 patch (obsolete) — Splinter Review
Attachment #8947821 - Flags: review?(pbrosset)
Comment on attachment 8947821 [details] [diff] [review]
1428442 patch

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

Thanks Yura. Will forward this to Greg who has been working on this code last.
Attachment #8947821 - Flags: review?(pbrosset) → review?(gtatum)
Comment on attachment 8947821 [details] [diff] [review]
1428442 patch

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

Looks like a pretty straightforward patch.

Please update the browser_toolbox_highlight.js to take into account the new behavior, and then I'll be comfortable with an r+.

Thanks!

devtools/client/framework/test/browser_toolbox_highlight.js

::: devtools/client/framework/components/toolbox-tab.js
@@ +46,5 @@
>  
>      const className = [
>        "devtools-tab",
>        currentToolId === id ? "selected" : "",
> +      highlightedTools && highlightedTools.has(id) ? "highlighted" : "",

You could make this line simpler by having highlightedTools non-optional in the PropTypes, but I'll leave it up to you with what you want to do.

::: devtools/client/framework/components/toolbox-toolbar.js
@@ +26,5 @@
>        // List of command button definitions.
>        toolboxButtons: PropTypes.array,
>        // The id of the currently selected tool, e.g. "inspector"
>        currentToolId: PropTypes.string,
> +      // An optionally highlighted tools, e.g. "inspector"

I would like to see this comment mention that this prop is a Set, since it's a bit ambiguos when declared as an PropTypes.object.
Attachment #8947821 - Flags: review?(gtatum) → review-
Attached patch 1428442 patch v2Splinter Review
Updated test, addressed comments, thanks!
Attachment #8947821 - Attachment is obsolete: true
Attachment #8948515 - Flags: review?(gtatum)
Comment on attachment 8948515 [details] [diff] [review]
1428442 patch v2

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

Looks great, thanks!
Attachment #8948515 - Flags: review?(gtatum) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d719873a21a9
enabling tab highlighting for more than one tool at a time. r=gregtatum
https://hg.mozilla.org/mozilla-central/rev/d719873a21a9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.