Refactor HighlightersOverlay and TooltipsOverlay into separate modules

RESOLVED FIXED in Firefox 52

Status

DevTools
Inspector
P3
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 52

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The purpose of this bug is to refactor style-inspector-overlays.js and separate out HighlightersOverlay and TooltipsOverlay into their own individual file/module. This will keep files smaller in size and easier to manage and edit. In addition, this will make it easier to add new features in HighlightersOverlay to take care of handling multiple grid highlighters on the page.
(Assignee)

Comment 2

2 years ago
Created attachment 8809740 [details] [diff] [review]
1316797.patch [1.0]
Attachment #8809740 - Flags: review?(jdescottes)
(Assignee)

Updated

2 years ago
Attachment #8809740 - Flags: review?(jdescottes)
(Assignee)

Comment 3

2 years ago
Created attachment 8809744 [details] [diff] [review]
1316797.patch [2.0]
Attachment #8809740 - Attachment is obsolete: true
Attachment #8809744 - Flags: review?(jdescottes)
Comment on attachment 8809744 [details] [diff] [review]
1316797.patch [2.0]

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

Nice refactor Gabriel! 
Works for me with my comments addressed and if try is green.

::: devtools/client/inspector/shared/highlighters-overlay.js
@@ +9,5 @@
>  // The style-inspector overlays are:
>  // - tooltips that appear when hovering over property values
>  // - editor tooltips that appear when clicking color swatches, etc.
>  // - in-content highlighters that appear when hovering over property values
>  // - etc.

Update this comment and remove references to the tooltip overlays.

@@ +47,5 @@
>    /**
>     * Add the highlighters overlay to the view. This will start tracking mouse
>     * movements and display highlighters when needed
>     */
> +  addToView() {

Not against the shorthand style, but it's inconsistent with the rest of the files in this folder. Unless there is a reason to use it only here, I would switch back to the previous style.

::: devtools/client/inspector/shared/tooltips-overlay.js
@@ +8,5 @@
>  
>  // The style-inspector overlays are:
>  // - tooltips that appear when hovering over property values
>  // - editor tooltips that appear when clicking color swatches, etc.
>  // - in-content highlighters that appear when hovering over property values

Update this comment and remove references to the highlighter overlays.
Attachment #8809744 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8809844 [details] [diff] [review]
1316797.patch [3.0]

Fixed comments and reverted shorthands.
Attachment #8809744 - Attachment is obsolete: true
Attachment #8809844 - Flags: review+
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3530eb420c7f2ec8510304e7e30aee8fe07c23e7
Bug 1316797 - Refactor HighlightersOverlay and TooltipsOverlay into separate modules r=jdescottes

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3530eb420c7f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.