Add colour swatches for the borders to the box model

RESOLVED WONTFIX

Status

()

Firefox
Developer Tools: Inspector
RESOLVED WONTFIX
4 years ago
3 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8379938 [details] [diff] [review]
patch2

This applies on top of the patch in bug 663778 and the patch in bug 850336
(Assignee)

Updated

4 years ago
Depends on: 850336
(Assignee)

Comment 2

4 years ago
Comment on attachment 8379938 [details] [diff] [review]
patch2

Ditto here
Attachment #8379938 - Flags: feedback?(pbrosset)
Comment on attachment 8379938 [details] [diff] [review]
patch2

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

Some of the feedback I gave in bug 850336 apply here too (especially in the test file).
I don't have a whole lot of feedback on the code of this bug. It's pretty straightforward, and it looks like the Toolip's colorswatch API was pretty well suited to the task here.

I'm not entirely convinced about the functionality itself though. It feels to me like duplicating a bit what you can already do in the rule-view. For me the layout-view is about distances, and although bug 850336 definitely makes sense, I'm not so sure about editing colors. What about border style then?
I'm happy to get convinced otherwise if there's a need for this but so far, I think this is making a mostly read-only panel more complex for something you can already do in the rule-view which, I think, is more natural for this kind of changes.
This is just my point of view anyway.

::: browser/devtools/layoutview/view.js
@@ +383,5 @@
> +        let ucfirst = position.substring(0, 1).toUpperCase() + position.substring(1);
> +        let swatch = document.querySelector(this.map["border" + ucfirst].swatch);
> +        let color = computed["border-" + position + "-color"].value;
> +        let style = computed["border-" + position + "-style"].value;
> +        if (style == "hidden" || style == "none")

I'm not personally against not using {} for if/else, but I haven't seen this anywhere else in the devtools code, so better go with the prevailing style and add { and } around these.
Attachment #8379938 - Flags: feedback?(pbrosset)
(Assignee)

Comment 4

3 years ago
Yeah this is probably too much for the box model view I think. I'm not going to push on with this.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.