Issue 343343003: DevTools: [Styles] Add "px" suffix to numbers on arrow up/down

Issue 343343003: DevTools: [Styles] Add "px" suffix to numbers on arrow up/down (Closed)

Created:
6 years, 6 months ago by lushnikov

Modified:
6 years, 5 months ago

Reviewers:
apavlov, pfeldman

CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

DevTools: [Styles] Add "px" suffix to numbers on arrow up/down This patch adds functionality to proactively add "px" suffix if editing happens in certain CSS properties. List of properties where only distance of length values are allowed is taken from http://www.w3.org/TR/CSS21/propidx.html BUG=387884 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177050

Patch Set 1 #

Patch Set 2 : remove "border" and "outline" #

Total comments: 2

Patch Set 3 : address comments #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+42 lines, -12 lines)			Patch
M	Source/devtools/front_end/elements/MetricsSidebarPane.js	View		1 chunk	+8 lines, -2 lines	0 comments	Download
M	Source/devtools/front_end/elements/StylesSidebarPane.js	View	1 2	1 chunk	+15 lines, -1 line	0 comments	Download
M	Source/devtools/front_end/sdk/CSSMetadata.js	View	1 2	2 chunks	+16 lines, -0 lines	0 comments	Download
M	Source/devtools/front_end/ui/UIUtils.js	View		3 chunks	+3 lines, -9 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

apavlov

lgtm https://codereview.chromium.org/343343003/diff/20001/Source/devtools/front_end/elements/StylesSidebarPane.js File Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/343343003/diff/20001/Source/devtools/front_end/elements/StylesSidebarPane.js#newcode3297 Source/devtools/front_end/elements/StylesSidebarPane.js:3297: if (!suffix.length && WebInspector.CSSMetadata.isLengthProperty(this._sidebarPane.property.name)) please don't require the ...

6 years, 6 months ago (2014-06-24 15:22:27 UTC) #2

lushnikov

Thanks! https://codereview.chromium.org/343343003/diff/20001/Source/devtools/front_end/elements/StylesSidebarPane.js File Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/343343003/diff/20001/Source/devtools/front_end/elements/StylesSidebarPane.js#newcode3297 Source/devtools/front_end/elements/StylesSidebarPane.js:3297: if (!suffix.length && WebInspector.CSSMetadata.isLengthProperty(this._sidebarPane.property.name)) On 2014/06/24 15:22:27, apavlov ...

6 years, 6 months ago (2014-06-24 15:39:46 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lushnikov@chromium.org/343343003/40001

6 years, 6 months ago (2014-06-26 12:10:55 UTC) #5

spicyj

If I'm reading this properly, it looks like this will add `px` to line-height as ...

6 years, 5 months ago (2014-06-30 18:23:07 UTC) #7

lushnikov

6 years, 5 months ago (2014-07-01 07:55:19 UTC) #8

Message was sent while issue was closed.

On 2014/06/30 18:23:07, spicyj wrote:
> If I'm reading this properly, it looks like this will add `px` to line-height
as
> well, which seems undesirable.

Indeed, that's undesirable. A follow-up patch fixes this:
https://codereview.chromium.org/361873003/
Thank you!

Also, could you please next time leave comments like this in the attached BUG if
any? They have more visibility there, thus less chances to be missed.

Expand Messages | Collapse Messages