[Webkit-unassigned] [Bug 226883] Web Inspector: add contextual documentation for CSS properties
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 1 12:03:51 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=226883
--- Comment #23 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 432655
--> https://bugs.webkit.org/attachment.cgi?id=432655
Patch v1.4 -Trimmed database of unwanted properties, console.assert() checks added, uniform icon sizing
View in context: https://bugs.webkit.org/attachment.cgi?id=432655&action=review
Looking good! Almost there =D
I think after these we'll be in a good place to land :)
> Source/WebInspectorUI/UserInterface/Base/Setting.js:215
> + showContextualSyntax: new WI.Setting("show-contextual-syntax", true),
I thought we agreed to default disabled?
also, I think this needs a more clear name like `showCSSPropertySyntaxInDocumentationPopover`
> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:21
> + #unavailable {
i think you can remove this now
> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:29
> + #selected {
This seems unused. Remove?
> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:33
> + #selected-active {
ditto :(29)
> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:46
> + <svg id="selected"><use xlink:href="#info"/></svg>
> + <svg id="selected-active"><use xlink:href="#info"/></svg>
ditto :(29)
> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:47
> + <svg id="unavailable"><use xlink:href="#noinfo"/></svg>
ditto (:21)
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:53
> +.sidebar > .panel.details.css-style > .content > .computed .computed-style-variables .property .go-to-arrow {
`.sidebar > .panel.details.css-style > .content > .computed > .details-section.computed-style-variables .property .go-to-arrow`
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:59
> +.sidebar > .panel.details.css-style > .content > .computed .computed-property-item .property .go-to-arrow {
The styles containing `.computed-property-item` should be moved to `ComputedStyleSection.css` as that's where all of the other `.computed-property-item` styles are.
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:66
> +.sidebar > .panel.details.css-style > .content > .computed .computed-property-item:not(:hover) > .property .go-to-arrow,
> .sidebar > .panel.details.css-style > .content > .computed .property:not(:hover) .go-to-arrow {
This doesn't quite work the way I'd expect it. It causes the go-to arrow to appear anywhere when hovering anywhere on the line in the Variables section, but not the Properties section. I think you need something more like this
```
.sidebar > .panel.details.css-style > .content > .computed > .details-section.computed-style-variables .property:not(:hover) .go-to-arrow
```
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:70
> +.computed-property-item > .property > .content > .contextual-documentation-button {
This should also have `.sidebar > .panel.details.css-style > .content > .computed` to match all of the above.
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:76
> +.computed-property-item:not(:hover) > .property > .content > .contextual-documentation-button {
ditto (:70)
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:26
> + .popover .documentation-popover-content {
Style: unnecessary leading space
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:26
> +WI.ContextualDocumentationPopover = class ContextualDocumentationPopover
I just realized that this doesn't actually inherit from `WI.Popover`. That's what all the other `WI.*Popover` classes do.
```
WI.ContextualDocumentationPopover = class ContextualDocumentationPopover extends WI.Popover
{
constructor(cssProperty, delegate)
{
console.assert(cssProperty instanceof WI.CSSProperty, cssProperty);
super(delegate);
this._cssProperty = cssProperty;
this._targetElement = null;
this.windowResizeHandler = this._presentOverTargetElement.bind(this);
}
// Public
show(targetElement)
{
this._targetElement = targetElement;
let documentationDetails = this._getDocumentationDetails(this._cssProperty);
let documentationElement = this._createDocumentationElement(documentationDetails);
this._popover.content = documentationElement;
this._presentOverTargetElement();
}
// Private
_presentOverTargetElement()
{
if (!this._targetElement)
return;
let targetFrame = WI.Rect.rectFromClientRect(this._targetElement.getBoundingClientRect());
this.present(targetFrame.pad(2), [WI.RectEdge.MIN_X, WI.RectEdge.MIN_Y, WI.RectEdge.MAX_Y]);
}
...
```
This structure will make it so that if the Web Inspector window is resized, the popover will remain in the right spot. It's also a more familiar pattern to those who've worked with other `WI.Popover` subclasses elsewhere in Web Inspector. (And it better matches the fact that this has the word "Popover" in its name)
You'll need to adjust the callsites as well
```
_presentContextualDocumentation()
{
this._contextualDocumentationPopover ??= new WI.ContextualDocumentationPopover(this._property, this);
this._contextualDocumentationPopover.show(this._nameElement);
...
```
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:40
> + let documentationDetails = this._getDocumentationDetails(this._delegate.property);
This is a really odd way of using a delegate. Normally, in order to keep code more readable (e.g. "who is calling this function") we prefix any delegate function with the name of the thing calling it (e.g. `contextualDocumentationPopoverProperty`). Delegates are really meant to be more of a "this object can be _anything_ so long as it has these specifically named functions" and not some specific kind of object.
Given my comment on :26 tho, you'll likely change this anyways. This is more of a FYI :)
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:68
> + return details;
NIT: you could inline this whole object
```
let propertyDocumentation = ContextualDocumentationDatabase[propertyName];
console.assert(propertyDocumentation, propertyName, ContextualDocumentationDatabase);
return {
name: propertyName,
description: propertyDocumentation.description,
syntax: propertyDocumentation.syntax,
url: propertyDocumentation.url,
};
```
> Source/WebInspectorUI/UserInterface/Views/Main.css:350
> + vertical-align: bottom;
This doesn't look good in the Styles panel.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:91
> +.spreadsheet-style-declaration-editor > .property:not(:hover) > .content .contextual-documentation-button,
NIT: this could/should have a `>` between `.content` and `.contextual-documentation-button`
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:108
> + willDismissPopover()
Style: this should be moved to a new `// Popover delegate` "section" (just like how there's one for `// SpreadsheetTextField delegate`) right above `// Private`
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:548
> +
> +
Style: unnecessary newlines
--
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/20210701/0d39c7f8/attachment.htm>
More information about the webkit-unassigned
mailing list