Closed Bug 974639 Opened 10 years ago Closed 10 years ago

Add colour swatches for the borders to the box model

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file)

      No description provided.
Attached patch patch2Splinter Review
This applies on top of the patch in bug 663778 and the patch in bug 850336
Depends on: 850336
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)
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
Closed: 10 years ago
Resolution: --- → WONTFIX
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.