[webkit-reviews] review granted: [Bug 178354] Web Inspector: [PARITY] Styles Redesign: Add color picker inline widget : [Attachment 323974] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 16 21:51:31 PDT 2017
Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 178354: Web Inspector: [PARITY] Styles Redesign: Add color picker inline
widget
https://bugs.webkit.org/show_bug.cgi?id=178354
Attachment 323974: Patch
https://bugs.webkit.org/attachment.cgi?id=323974&action=review
--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 323974
--> https://bugs.webkit.org/attachment.cgi?id=323974
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=323974&action=review
r=me, my comments are just style and nits.
> Source/WebInspectorUI/UserInterface/Models/Color.js:645
> + "hsla"
Style: Include the trailing comma. It makes future diffs nicer. We might add a
color() for p3 later.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:285
> + let colorTokenElement = document.createElement("span");
> + colorTokenElement.classList.add("token-color");
> +
> + let innerElement = document.createElement("span");
> + innerElement.classList.add("token-color-value");
Style: For elements we create where we are setting the class names ourselves I
think we should use `elem.className = x` instead of `elem.classList.add(x)`. So
here:
colorTokenElement.className = "token-color";
...
innerElement.className = "token-color-value";
In practice it probably doesn't matter, but it is slightly faster and easier to
reason about (not having to worry about other potential class names).
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:296
> + let swatch = event && event.target;
> + console.assert(swatch, "Color swatch is missing.");
> + if (!swatch)
> + return;
This is not possible. If the event happens `event` and `event.target` cannot be
missing.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:310
> + swatch.element.addEventListener("mousedown", (event) =>
event.stop());
Style: We prefer using braces with arrow functions if the return value is
unimportant / unused. So:
swatch.element.addEventListener("mousedown", (event) => { event.stop(); });
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:351
> + let possiblyColor =
this._stringFromTokens(tokens.slice(colorFunctionStartIndex, i + 1));
> + let color = WI.Color.fromString(possiblyColor);
> + if (color)
> + newTokens.push(createColorTokenElement(possiblyColor,
color));
> + else
> + newTokens.push(...tokens.slice(colorFunctionStartIndex,
i + 1));
Nit: `possiblyColor` => `possibleColor`
Nit: You should stash the slice into a local to avoid re-computing it later if
the fromString fails:
let rawTokens = tokens.slice(colorFunctionStartIndex, i + 1);
let possibleColor = this._stringFromTokens(rawTokens);
let color = WI.Color.fromString(possibleColor);
if (color)
newTokens.push(createColorTokenElement(possibleColor, color));
else
newTokens.push(...rawTokens);
Finally, in general these blocks can share some code:
function pushPossibleColorToken(text, ...tokens) {
let color = WI.Color.fromString(text);
if (color)
newTokens.push(createColorTokenElement(text, color);
else
newTokens.push(...tokens);
}
for (let i = 0; i < tokens.length; ++i) {
let token = tokens[i];
if (...) {
// Hex
pushPossibleColorToken(token.value, token);
} else if (...) {
// Color Function start
...
} else if (...) {
// Nickname
pushPossibleColorToken(token.value, token);
} else if (...) {
// Color Function end
let rawTokens = tokens.slice(colorFunctionStartIndex, i + 1);
let text = this._stringFromTokens(rawTokens);
pushPossibleColorToken(text, rawTokens);
colorFunctionStartIndex = NaN;
}
}
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:366
> + return tokens.map((token) => {
> + return token.value;
> + }).join("");
Nit: Simplify:
return tokens.map((token) => token.value).join("");
More information about the webkit-reviews
mailing list