[webkit-reviews] review granted: [Bug 226883] Web Inspector: add contextual documentation for CSS properties : [Attachment 432738] Patch v1.5 - ContextualDocumentationPopover inherits WI.Popover, removed unwanted code from infoIcon.svg, moved css to more specific files

Thu Jul 1 16:14:15 PDT 2021

Devin Rousso <drousso at apple.com> has granted Harshil Ratnu <hratnu at apple.com>'s
request for review:
Bug 226883: Web Inspector: add contextual documentation for CSS properties
https://bugs.webkit.org/show_bug.cgi?id=226883

Attachment 432738: Patch v1.5 - ContextualDocumentationPopover inherits
WI.Popover, removed unwanted code from infoIcon.svg, moved css to more specific
files

https://bugs.webkit.org/attachment.cgi?id=432738&action=review

--- Comment #26 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 432738
  --> https://bugs.webkit.org/attachment.cgi?id=432738
Patch v1.5 - ContextualDocumentationPopover inherits WI.Popover, removed
unwanted code from infoIcon.svg, moved css to more specific files

View in context: https://bugs.webkit.org/attachment.cgi?id=432738&action=review

r=me, nice work!  A few remaining Style/NIT comments, but nothing too serious
:)

> Source/WebInspectorUI/UserInterface/Base/Setting.js:215
> +    showCSSPropertySyntaxInDocumentationPopover: new
WI.Setting("show-css-property-syntax-in-documentation-popover", false),

Style: this should go before anything else `showC*` to be alphabetical (capital
< lowercase)

> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:28
> +    <path id="noinfo" d="M5,10 C7.761,10 10,7.761 10,5 C10,2.239 7.761,0 5,0
C2.239,0 -1.77635684e-15,2.239 -1.77635684e-15,5 C-1.77635684e-15,7.761
2.239,10 5,10 Z M1.500,4.250 L8.500,4.250 L8.500,5.750 L1.500,5.750
L1.500,4.250 Z"/>

this isn't used

> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:32
> +  <svg id="normal"><use xlink:href="#info"/></svg>
> +  <svg id="active"><use xlink:href="#info"/></svg>

Thinking about this a bit more, I think you can actually simplify this by just
having `fill="currentColor"`
```
    <?xml version="1.0" encoding="utf-8"?>
    <!-- Copyright © 2021 Apple Inc. All rights reserved. -->
    <svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10">
	<path fill="currentColor" d="M5,10 C7.761,10 10,7.761 10,5 C10,2.239
7.761,0 5,0 C2.239,0 0,2.239 0,5 C0,7.761 2.239,10 5,10 Z M5.182,3.790
C5.375,3.790 5.528,3.855 5.632,3.986 C5.711,4.085 5.759,4.206 5.774,4.348
L5.780,4.458 L5.779,7.278 L6.374,7.278 C6.467,7.278 6.552,7.297 6.629,7.333
L6.703,7.376 L6.771,7.432 C6.879,7.535 6.934,7.666 6.934,7.819 C6.934,7.976
6.879,8.110 6.771,8.214 C6.684,8.295 6.581,8.344 6.464,8.361 L6.374,8.367
L3.952,8.367 C3.797,8.367 3.662,8.315 3.555,8.213 C3.447,8.110 3.392,7.976
3.392,7.819 C3.392,7.666 3.447,7.535 3.555,7.432 C3.640,7.350 3.744,7.301
3.861,7.284 L3.952,7.278 L4.571,7.278 L4.571,4.879 L4.053,4.879 C3.962,4.879
3.878,4.860 3.801,4.824 L3.728,4.781 L3.660,4.726 C3.549,4.623 3.492,4.489
3.492,4.332 C3.492,4.178 3.549,4.046 3.660,3.943 C3.747,3.862 3.849,3.813
3.964,3.796 L4.053,3.790 L5.182,3.790 Z M4.955,1.230 C5.205,1.230 5.419,1.318
5.590,1.493 C5.763,1.667 5.849,1.884 5.849,2.137 C5.849,2.382 5.762,2.596
5.591,2.771 C5.419,2.948 5.205,3.038 4.955,3.038 C4.709,3.038 4.497,2.948
4.322,2.771 C4.148,2.596 4.060,2.381 4.060,2.137 C4.060,1.884 4.148,1.668
4.322,1.492 C4.497,1.318 4.709,1.230 4.955,1.230 Z"/>
    </svg>
```
and then having the usage also have CSS `color: hsla(0, 0%, 0%, 0.5)`.	Not
sure if that would work with CSS `background-image` tho ��

>
Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:43
> +    font-size: 10.2px;

`10.2px`?  Why not just `10px`?

>
Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:80
> +	       url: propertyDocumentation.url

Style: missing ending `,`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528
> +	   if
(ContextualDocumentationDatabase.hasOwnProperty(this._property.name) ||
ContextualDocumentationDatabase.hasOwnProperty(this._property.canonicalName)) {

NIT: I'd make this into an early return