Summary: | Web Inspector: [PARITY] Styles Redesign: Add color picker inline widget | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 178404 | ||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2017-10-16 11:46:03 PDT
Created attachment 323974 [details]
Patch
Created attachment 323975 [details] [Image] With patch applied Static page for manual testing: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/color.html Comment on attachment 323974 [details] 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(""); One thing our old color swatch thing did is detect if this was a color inside of a gradient definition and didn't include a color swatch for those. I assume that is something that you'll address when adding a gradient swatch. But it might be worth a fixme so it doesn't get forgotten about. Created attachment 324047 [details]
Patch
Comment on attachment 324047 [details] Patch Clearing flags on attachment: 324047 Committed r223575: <https://trac.webkit.org/changeset/223575> All reviewed patches have been landed. Closing bug. |