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

Fri Jun 25 13:49:36 PDT 2021

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

--- Comment #10 from Harshil Ratnu <hratnu 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?

Since the ContextualDocumentationButton is in-line with the rest of the text it's size needs to be restricted to be so small. If these icons are any bigger then when the user hovers on top of any line where they should appear, that makes the particular line pop-up a little visually as it gets a little extra height.

>> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:18
>> +        fill: hsla(0, 0%, 0%, 0.5);
> 
> Does this look good in dark mode?

Yes the styling is made the same as go-to-arrow button in the computed panel and it works with dark mode equally well.

>> Source/WebInspectorUI/UserInterface/Main.html:681
>> +    <script src="Views/ContextualDocumentationDB.js"></script>
> 
> Where is this file?

In this patch: didn't show up sorry. Will add to next version of this patch.
Originally: Git Repository -  VSCodeCustomDataCSS
Locally:Should be present in the External Folder in a future patch. Working on it.

>> 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`.

Done. Used a more specific rule.

>> 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?

I did not know the 0.5 increment rule. 1.3 makes it perfectly inline but in 1.5 either the difference isn't noticeable hence changed to that for now.

>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:101
>> +.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`).

A lot of this code was copies over from go-to-arrow button. In the next patch this section has been cleared significantly to leave only the relevant code.

>> 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([ ... ]);`

Working on preprocessing data that will be part of the update script(to update this database from time to time) in a future patch.

>> 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?

Beautiful Suggestion! This is now handled in WI.SpreadsheetStyleProperty.prototype.presentContextualDocumentation  and the getter has been removed.

The behavior is such that the popover will only persist if the user clicked in the value field of that property which is what we want so that the user has reference to the syntax from the popover while editing the value. Clicking anywhere else will dismiss the popover.

>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:125
>> +            syntaxBodyElement.textContent = `:  ${details.syntax}`; 
> 
> Why two spaces after the `:`?
> 
> Also, based on the demo video (attachment 431903 [details]), 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.

two spaces: typo sorry.

implementation detail: You are right the syntax in some cases is too complicated to be useful to the everyday user. However with the limited time that we have(Internship ends next week) in this and the limitations that the current data source presents, we cannot possible change the documentation quality. This would however be a great update in the future when the documentation quality can be given more time.

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

Old Design. Fixed in next patch.

>> 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.

Localization added in next patch.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:555
>> +            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

Thanks! Fixed.

-- 
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/20210625/7aedaef5/attachment-0001.htm>