[Webkit-unassigned] [Bug 226883] Web Inspector: add contextual documentation for CSS properties
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 28 14:18:46 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=226883
--- Comment #13 from Harshil Ratnu <hratnu 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
Yup. Fixed.
>> 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.
Sorry I still would like to defend this feature. This is the same syntax VS Code shows in their IDE and also the same syntax that Firefox used to show in their Nightly Browser at some point.
To the developers who would understand complex syntax values it still means something and provides valueable quick reference. And in cases where it is not that useful there is always the option to click on MDN Reference to go to the actual page and read more. Moreover there are a lot of scernarios where the syntax is really useful and hence we should focus on those upsides. No Documentation is perfect but we still must provide whatever information we can and hopefully one day we can improve this documentation quality and makes it easier to understand. In the time being, if we realize via telemetry or other sources that syntax is not a helpful field to our users, we can always just comment out the line in code responsible for adding it to the popover. I do not favor removing it entirely from this feature as this has been a core part of this project and everyone in the process of development and demos has shown keen interest in this detail.
>> 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`).
GoToArrow.svg has the same size and since this sits right next to the go-to-arrow in computed panel it was made the same.
>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:59
>> + margin-left: 2px;
>
> Why did this change?
Go to Arrow was not in line with the text in computed panel, It was slightly higher vertically
>> 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 `>`
Do we need :matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) ? If we are okay specifying It as .property > .content> .contextual-documentation-button { ... } then we don't. We can also separate them out in their individual files like
````
in SpreadsheetCSSStyleDeclarationEditorcss
.spreadsheet-style-declaration-editor.properties > .property > .content > .contextual-documentation-button {
display: none;
}
in ComputedStylesSection.css
.computed-property-item > .property > .content > .contextual-documentation-button {
display: none;
}
```````````
However in that case any change in behavior for this button will have to be made in both files always. To avoid that I have placed this code entirely in SpreadsheetCSSStyleDeclarationEditor.css for now. Kindly let me know if you would have it otherwise.
>> 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.
Increasing the size more than 10*10 makes the hover action jumpy.
>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:41
>> + for (let prop of ContextualDocumentationDatabase.properties) {
>
> I thought there was a plan to preprocess this?
Planning on doing that in the next patch. Update Script was taking some time and I wanted feedback on other things in the meantime.
>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:88
>> + 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);`)?
Its not a possible path as then the addInfoButton would not have appeared and we wouldn't have searched for the property to begin with.
>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:126
>> + referenceLinkElement.textContent = "MDN Reference";
>
> this should probably be a `WI.UIString`
The link leads to untranslated documentation on MDN.
Likewise, all of the tooltip content is in English.
This sets the expectation for the user that they’re not going to get something in their own language at the destination.
>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:524
>> + if (!WI.CSSCompletions.cssNameCompletions.isValidPropertyName(this._property.name))
>
> Do we also want to check `this._property.canonicalName`?
I believe the function isValidPropertyName doesn't need that check and already accounts for that as this is the same conditional that we use for painting the warning sign at the end of line if the propertyNameIsNotValid
>> 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?
Maciej requested this. Originally we weren't showing any button at all for the properties where we do not have a documentation and where the property name was invalid. But Maciej wanted us to still have a button if the property was valid but we did not have it's documentation so as to differentiate the behavior and preventing the user from thinking that they did a mistake hence they can't see a button. He then in the second demo looked over this design and approved it.
--
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/20210628/6f1eb86e/attachment-0001.htm>
More information about the webkit-unassigned
mailing list