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)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: mossop, Assigned: mossop)
References
Details
Attachments
(1 file)
20.97 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
This applies on top of the patch in bug 663778 and the patch in bug 850336
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8379938 [details] [diff] [review] patch2 Ditto here
Attachment #8379938 -
Flags: feedback?(pbrosset)
Comment 3•10 years ago
|
||
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•10 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
Closed: 10 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•