Closed
Bug 1456681
Opened 7 years ago
Closed 6 years ago
Toggle the flexbox highlighter from the markup badges
Categories
(DevTools :: Inspector, enhancement, P3)
DevTools
Inspector
Tracking
(firefox63 fixed)
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: gl, Assigned: gl)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Updated•6 years ago
|
Product: Firefox → DevTools
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8989024 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.
https://reviewboard.mozilla.org/r/254112/#review260834
Code analysis found 1 defect in this patch:
- 1 defect found by mozlint
You can run this analysis locally with:
- `./mach lint path/to/file` (JS/Python)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: devtools/client/inspector/markup/views/element-editor.js:662
(Diff revision 1)
> }
>
> - this.highlighters.toggleGridHighlighter(this.inspector.selection.nodeFront, "markup");
> + if (target.dataset.display === "grid" || target.dataset.display === "inline-grid") {
> + this.highlighters.toggleGridHighlighter(this.inspector.selection.nodeFront,
> + "markup");
> + return;
Error: Unnecessary return statement. [eslint: no-useless-return]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8989024 -
Attachment is obsolete: true
Attachment #8989024 -
Flags: review?(pbrosset)
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8989033 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.
https://reviewboard.mozilla.org/r/254118/#review260912
Looks good in general, but this should be hidden behind the flexbox highlighter pref. If you have it off, then the badge should not be clickable, for consistency.
Attachment #8989033 -
Flags: review?(pbrosset)
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8989033 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.
https://reviewboard.mozilla.org/r/254118/#review261226
::: devtools/client/themes/markup.css:421
(Diff revision 2)
> +.markup-badge[data-display="flex"],
> .markup-badge[data-display="grid"],
> +.markup-badge[data-display="inline-flex"],
> +.markup-badge[data-display="inline-grid"],
> .markup-badge[data-event] {
> cursor: pointer;
> }
>
> +.markup-badge[data-display="flex"]:focus,
> +.markup-badge[data-display="flex"]:hover,
> .markup-badge[data-display="grid"]:focus,
> .markup-badge[data-display="grid"]:hover,
> +.markup-badge[data-display="inline-flex"]:focus,
> +.markup-badge[data-display="inline-flex"]:hover,
> +.markup-badge[data-display="inline-grid"]:focus,
> +.markup-badge[data-display="inline-grid"]:hover,
> .markup-badge[data-event]:focus,
> .markup-badge[data-event]:hover {
> background-color: var(--markup-badge-hover-background-color);
> }
I'm just a bit concerned that this will make the badge appear clickable even when clicking it doesn't do anything (most users have and will continue to have the pref off).
The simplest solution I see here to land this patch quickly would be to add a class to the badge. Either something that expresses that it can't be clicked (e.g. `inactive`) so you can then add `:not(.inactive)` to those selectors.
Or the opposite, add a class to all badges that *are* clickable (e.g. `interactive`), so you can then add this class to all badges that can be clicked (like the event bubble too).
The advantage of adding a class is you can do that from JS, based on the value of the pref.
Attachment #8989033 -
Flags: review?(pbrosset)
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8989033 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.
https://reviewboard.mozilla.org/r/254118/#review261662
Attachment #8989033 -
Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13209d305efe
Toggle the flexbox highlighter from the markup display badges. r=pbro
Comment 10•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in
before you can comment on or make changes to this bug.
Description
•