[Webkit-unassigned] [Bug 226883] Web Inspector: add contextual documentation for CSS properties
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 25 17:15:13 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=226883
--- Comment #12 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 432292
--> https://bugs.webkit.org/attachment.cgi?id=432292
Patch v1.2 -Localizatiion, Database moved in External Folder, Other refactoring and styling changes
View in context: https://bugs.webkit.org/attachment.cgi?id=432292&action=review
> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:186
> +my $shouldCombineMain = 1; #defined $ENV{'COMBINE_INSPECTOR_RESOURCES'} && ($ENV{'COMBINE_INSPECTOR_RESOURCES'} eq 'YES');
oops
> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:47
> + "syntax": "normal | <baseline-position> | <content-distribution> | <overflow-position>? <content-position>",
I'm still not convinced that this is a good approach. Perhaps for now we should remove this (or put it behind an experimental feature) until we can come up with a better way to express this information.
> 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">
My point about this being small was more about the fact that other icons used in the Styles panel are 15×15 (e.g. `CubicBezier.svg`).
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:59
> - position: absolute;
> - bottom: 0;
> - width: var(--disclosure-button-size);
> - height: var(--disclosure-button-size);
> + width: 10px;
> + height: 10px;
> + position: relative;
> + z-index: 1;
> + top: 1.5px;
> + margin-left: 2px;
Why did this change?
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:55
> +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content> .contextual-documentation-button {
Do we actually need `:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content >`? `.contextual-documentation-button` is already a unique name, so there shouldn't be any need for more a more specific selector.
What I was trying to describe earlier was that if you have any _additional_ modifications from the base style that are specific for some other circumstance, those modifications should go in the file most closely related to that. To give an example here, the base style `.contextual-documentation-button` shouldn't have any `display` and instead you should move those styles to `Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleDeclarationEditor.css` and/or `Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css` so that if we decide to create a `WI.ContextualDocumentationButton` that's _always_ visible elsewhere in Web Inspector then these styles won't be relevant/necessary.
Style: space before `>`
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:64
> + background-color: transparent;
> + background-repeat: no-repeat;
> + background-position: center;
> + background-size: 10px 10px;
You can replace all of these with just a `background: none`.
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:69
> + top: 1.5px;
Does this work on 1x/non-retina screens? I don't think it will, so I'd recommend making this
```
width: 11px;
height: 11px;
top: 1px;
vertical-align: -1px;
```
That's similar to what we do with other things.
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:73
> +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content:hover> .contextual-documentation-button {
Can we make this so that hovering over the empty space next to the property also shows the button? I think that's possible by moving the `.content:hover` to `.property:hover` instead.
ditto (:73)
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:77
> +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content> .contextual-documentation-button:active {
ditto (:73)
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:85
> +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content> .contextual-documentation-button.unavailable {
ditto (:73)
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:94
> + .contextual-documentation-button{
Style: space before `{`
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:26
> +WI.ContextualDocumentation = class ContextualDocumentation {
This needs a better name that clarifies what it does, like `WI.ContextualDocumentationPopover`. Right now, this reads like a model object, not a view object.
Style: `{` on new line
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:29
> + this._field = delegate;
This should be called `_delegate` to match the name of the parameter.
Also, as a general piece of feedback, `_field` is too generic and doesn't really explain what is expected.
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:41
> + for (let prop of ContextualDocumentationDatabase.properties) {
I thought there was a plan to preprocess this?
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:88
> + if (property.canonicalName in propertyMap)
> + propertyName = property.canonicalName;
> + else if (property.name in propertyMap)
> + propertyName = property.name;
What happens if neither is in the `propertyMap`? Or is that not a possible path (if so we should `console.assert(propertyName);`)?
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:112
> + let syntaxPara = documentationElement.appendChild(document.createElement("p"));
> + syntaxPara.className = "syntax";
Style: we don't abbreviate variable names like this
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:126
> + referenceLinkElement.textContent = "MDN Reference";
this should probably be a `WI.UIString`
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:515
> + // This makes sure we don't add a button to line that already had it.
This comment doesn't add any value if one just reads the below code. I'd remove it.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:519
> + if (this._contextualDocumentationButton && this._contentElement?.contains(this._contextualDocumentationButton)) {
> + this._contentElement.removeChild(this._contextualDocumentationButton);
> + this._contextualDocumentationButton = null;
> + }
You can simplify this.
```
if (this._contextualDocumentationButton) {
this._contextualDocumentationButton.remove();
this._contextualDocumentationButton = null;
}
```
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:524
> + if (!WI.CSSCompletions.cssNameCompletions.isValidPropertyName(this._property.name))
Do we also want to check `this._property.canonicalName`?
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528
> + this._contextualDocumentationButton = this._contentElement.appendChild(document.createElement("button"));
I just realized that `.contextual-documentation-button` is not actually related to a `WI.ContextualDocumentationButton` and is just something that's created for this class. As such, you should not have a special `Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css` file just for these styles. All those styles should really be inside `Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleDeclarationEditor.css` and/or `Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css` as that's where the other `.spreadsheet-style-property` styles are. We should keep CSS close together for JS that's similarly close together.
Also, since this happens in both the `if` and `else`, I'd pull this out above the `if` to avoid repeated code.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:530
> + this._contextualDocumentationButton.className = "contextual-documentation-button";
ditto (:528)
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:531
> + this._contextualDocumentationButton.addEventListener("mousedown", (event) => {
Why not `"click"`?
Also please make sure this is only for left-click as right now a right-click will trigger this too.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:541
> + this._contextualDocumentationButton = this._contentElement.appendChild(document.createElement("button"));
ditto (:528)
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:542
> + this._contextualDocumentationButton.title = WI.UIString("Documentation not available", "Tooltip for unavailable documentation @ Contextual Documentation Popup");
This isn't really very helpful for a developer. Why are we showing a button at all if we don't have documentation for it?
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:543
> + this._contextualDocumentationButton.className = "contextual-documentation-button unavailable";
based on my comment on line 528, you'd want to change this to `.classList.add("unavailable")`
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:549
> + this._contextualDocumentation = new WI.ContextualDocumentation(this);
This needs a better name (like `this._contextualDocumentationPopover`) just like how `WI.ContextualDocumentation` needs a better name.
This means we create a new `this._contextualDocumentationPopover` each time the developer clicks on the `this._contextualDocumentationButton`. I think you should use `this._contextualDocumentation ??= new WI.ContextualDocumentation(this);` so that we only create 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/20210626/bce2229b/attachment-0001.htm>
More information about the webkit-unassigned
mailing list