[Webkit-unassigned] [Bug 226883] Web Inspector: add contextual documentation for CSS properties

Thu Jun 24 16:12:20 PDT 2021

https://bugs.webkit.org/show_bug.cgi?id=226883

--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 432130
  --> https://bugs.webkit.org/attachment.cgi?id=432130
Patch v1.1 -Updated Styling, Uniform Nomenclature, Removed Nested Conditionals

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

> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:3
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10">

10×10 seems small.  We normally have 15×15 or 16×16 icons.  Why so small?

> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:18
> +        fill: hsla(0, 0%, 0%, 0.5);

Does this look good in dark mode?

> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:44
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Main.html:681
> +    <script src="Views/ContextualDocumentationDB.js"></script>

Where is this file?

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:32
> +.documentation-popover p {

We generally try to use immediate descendant combinators wherever possible to lessen the likelihood that future changes will clash with past changes.  As such, can this be `.documentation-popver > p`?

ditto with other selectors below

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:33
> +    margin-bottom: 5px !important;

We try to avoid using `!important` unless there is literally no other choice (and even then usually there is).  Please rework to not use `!important`.

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:47
> +    color:var(--syntax-highlight-boolean-color);   

Style: missing space after `:`

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:76
> +    top: 1.3px;

This seems oddly specific.  In almost all cases CSS doesn't really care about anything other than `0.5` increments.  Is there a reason you need `0.3` exactly?  Maybe a `vertical-align: middle;` (or something along those lines) would work instead?

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:101
> +:focus .selected .contextual-documentation-button,
> +.selected:focus .contextual-documentation-button {

Please be more specific as to the context of `:focus` and `.selected`.  Is it a `.property` or a `.tree-element` or what?  We don't want general CSS selectors like this because if a `.contextual-documentation-button` is added to other parts of Web Inspector that have a radically different structure it would cause (potentially undiscovered) problems.  A general rule of thumb is to only include styles that apply to the node in isolation in that node's file (e.g. `.contextual-documentation-button` relates to `ContextualDocumentation.css`) and have more specific usage styles in the CSS file corresponding to that usage (e.g. `SpreadsheetStyleProperty.css`).

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:111
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:118
> +        filter: unset;

We generally try to avoid using `unset` and instead prefer to just not set the style in the first place (e.g. only applying `filter: invert();` when not `.selected:focus`).

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:120
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:27
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:34
> +   // Static

Style: needs four spaces for indentation

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:42
> +        for (let prop of VSCodeCustomDataCSS.properties) {

Can we format this data ahead of time when it's imported into the WebKit repo?  I doubt it would take much python to do it.

Also, we should call this something else.  Where the data comes from is not important.  Ideally, if you do pre-formatting, you could just have the result be output into a file that contains `WI.ContextualDocumentation.propertyMap = new Map([ ... ]);`

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:43
> +            let temp = {}

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:46
> +            if (prop.hasOwnProperty("syntax")) {

Style: no `{` or `}` for single line control flow

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:50
> +            if (prop.hasOwnProperty("references")) {

ditto (:46)

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:73
> +        this._popover.present(bounds.pad(2), [WI.RectEdge.MIN_Y,WI.RectEdge.MIN_X]);

Style: missing space after `,`

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:74
> +        this._field.valueElement.classList.add("popover-ignore-auto-dismiss");

It's really odd that this is added in this file but then removed in another file.  That's probably not going to be clear to future engineers.  Why not just have this inside `WI.SpreadsheetStyleProperty.prototype.presentContextualDocumentation`?  This would also remove the need to expose `get valueElement`, which is kinda an implementation detail of `WI.SpreadsheetStyleProperty`.

Also, please use the named constant for this `WI.Popover.IgnoreAutoDismissClassName`.

Most importantly tho, this also means that clicking outside of the `WI.Popover` or scrolling will prevent it from dismissing.  Is that really what we want?

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:78
> +    dismiss(){

Style: `{` on new line

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:86
> +        let details = {};

Style: please move this further down closer to where it's actually used

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:87
> +        const propertyMap  = WI.ContextualDocumentation.getPropertiesMap()

ditto (:43)

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:98
> +        details.description = propertyMap[propertyName].description
> +        details.syntax = propertyMap[propertyName].syntax
> +        details.url = propertyMap[propertyName].url

ditto (:43)

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:107
> +        let nameElement  = documentationElement.appendChild(document.createElement("p"));

Style: extra space

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:112
> +        descriptionElement.className = "description-para";

Style: we don't abbreviate names like this

Also, no need to add a class if it's not used.

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:117
> +            let syntaxPara = documentationElement.appendChild(document.createElement("p"));
> +            syntaxPara.className = "syntax-para";

ditto (:112)

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:121
> +            syntaxTitleElement.textContent = `${details.name}`;

No need for a template string here.  You can just use ` = details.name;``.

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:124
> +            syntaxBodyElement.className  ="syntax-body";

No need to add a class if it's not used.

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:125
> +            syntaxBodyElement.textContent = `:  ${details.syntax}`; 

Why two spaces after the `:`?

Also, based on the demo video (attachment 431903), I'm not sure what value the actual CSS syntax would have for someone working with CSS.  To use `display` as an example, I don't think most CSS developers know what `<display-outside>` actually means and would likely immediately jump to MDN to find that answer.  If the goal of this is to help developers learn/understand CSS without having to leave Web Inspector, I think we should include definitions for these other keywords here too or even entirely replace the keyword with its definition inline.  Some things we could probably keep (e.g. `<length>`, `<percentage>`, etc.) but I think most of these kinds of specific keywords would not really be all that useful.

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:132
> +            referenceLinkElement.className = "reference-link"
> +            referenceLinkElement.href = details.url
> +            referenceLinkElement.textContent = "MDN Reference"

ditto (:43)

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:137
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:138
> +}    

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:140
> +WI.ContextualDocumentation.propertyMap = null;

Since nobody else is supposed to access `WI.ContextualDocumentation.propertyMap`, please make it "private" by prefixing with a `_`.
```
    WI.ContextualDocumentation._propertyMap = null;
```

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:109
> +    presentContextualDocumentation()

This can be "private" (add a `_` prefix and move to the `// Private` section) since it's not used outside of this object.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:315
> -            let propertyNameIsValid = false;
> +            let isPropertyNameValid = false;

These kinds of changes don't really add anything and just make it harder to track down regressions by adding another `git blame` hop.  Please undo.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:417
> +    {   

Style: unnecessary spaces

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:524
> +            this._contentElement.removeChild(this._contextualDocumentationButton)

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:529
> +            return

ditto (:524)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:531
> +        if (!WI.CSSCompletions.cssNameCompletions?.isValidPropertyName(this._property.name)) 

I don't think it's possible for `WI.CSSCompletions.cssNameCompletions` to still be `null` by this point as it's one of the first bits of data requested by Web Inspector from the inspected page.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:535
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:540
> +            this._contextualDocumentationButton.addEventListener("mousedown",(event)=>{

Style: missing space after `,`
Style: missing spaces before and after `=>`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:541
> +                

ditto (:535)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:542
> +                if(this._valueTextField?.editing){

Style: missing space after `if`
Style: missing space before `{`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:548
> +                this.presentContextualDocumentation(true);

Why the `true`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:549
> +            })

ditto (:524)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:550
> +            

ditto (:417)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:553
> +            this._contextualDocumentationButton.setAttribute("title","Documentation not available")

You can just do `.title = `.

This should be a `WI.UIString` so it can be localized for other languages.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:555
> +            this._contextualDocumentationButton.classList.add("contextual-documentation-button");
> +            this._contextualDocumentationButton.classList.add("unavailable");

Since you just created this element, you can just use `className` with a manually space separated string.
```
    this._contextualDocumentationButton.className = "contextual-documentation-button unavailable";
```
FYI `classList.add` supports multiple arguments at once

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210624/3861ac7f/attachment-0001.htm>