[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 21:54:04 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=226883
--- Comment #15 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 432421
--> https://bugs.webkit.org/attachment.cgi?id=432421
Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files
View in context: https://bugs.webkit.org/attachment.cgi?id=432421&action=review
> Source/WebInspectorUI/ChangeLog:19
> + Add Localizations for tooltip strings that are exposed to the user
It generally isn't necessary for /every/ file and function to have notes if note would be easily inferred by a future reader. In this case, changes to the localized strings file isn't really noteworthy, as the strings also appear below in your code, where they are more important. You can get rid of this note (but keep the file listed here).
> Source/WebInspectorUI/ChangeLog:22
> + Add details for the ContextualDocumentationDatabase from External Folder to build properly
Much like comments, it is generally preferable that these notes are complete sentences. I think for most of these, you are just missing the ending period. This note applied throughout the change log. I won't note it on every line to avoid being overly redundant.
> Source/WebInspectorUI/ChangeLog:25
> + Add documentation of all css properties in a JSON object. This file is sourced from VSCodeCustomData
NIT: CSS is a capitalized acronym.
> Source/WebInspectorUI/ChangeLog:29
> + Add the LICENSE File for VSCode's data
ditto :19
> Source/WebInspectorUI/ChangeLog:32
> + Add a file for the new info button
ditto :19
> Source/WebInspectorUI/ChangeLog:35
> + Link the new files added to be loaded from Main.html
ditto :19
> Source/WebInspectorUI/ChangeLog:43
> + Add styling specific to the documentation buttons of Computed Panel
Style: Only need one space between `specific` and `to`.
> Source/WebInspectorUI/ChangeLog:52
> + Define styling for content within the documentation popover
ditto :19 - Unlike the targeted changes you made to existing stylesheets above that warranted some explanation, in my opinion it is inferred by the naming of the file and selectors what this spreadsheet will do.
> Source/WebInspectorUI/ChangeLog:68
> + Add all basics styles for the contextualDocumentationButton
ditto :52
> Source/WebInspectorUI/ChangeLog:84
> + Responsible for adding the info button add the end of the property.
NIT: `... next value field and is responsible for adding...`?
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:327
> +/* Tooltip to show documentation @ Contextual Documentation Popup */
> +localizedStrings["Click to show documentation"] = "Click to show documentation";
Generally the key for the string will be in the format of `Text @ Location in interface`, e.g. `Click to show documentation @ Contextual Documentation Popover Button`. The comment for the string can then be a bit more freeform e.g. `Tooltip for the button to show contextual documentation for CSS properties.`
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:506
> +/* Tooltip for unavailable documentation @ Contextual Documentation Popup */
> +localizedStrings["Documentation not available"] = "Documentation not available";
ditto :326-327
> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:2
> + "-moz-animation": {
Do we need the `-moz-`, `-ms-`, and `-o-` prefixed properties since I don't believe we would ever show documentation for those, only the standard documentation? I think `-webkit-` is the only prefix you need.
> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:691
> + "-webkit-background-composite": {},
Is there any value in keeping empty properties in the processed data, since we shouldn't show documentation in this case anyways? If not, let's remove it as well.
(I came back here after finishing review the rest, and it does look like this will be an issue currently. I'd recommend removing empty properties entirely so you correctly don't show documentation for them.)
> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:3248
> +};
Thank you for alphabetizing these entries!
> Source/WebInspectorUI/UserInterface/Main.html:93
> + <link rel="stylesheet" href="Views/ContextualDocumentationPopover.css">
Style: Please place this in alphabetical order.
> Source/WebInspectorUI/UserInterface/Main.html:682
> + <script src="Views/ContextualDocumentationPopover.js"></script>
ditto :93
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:55
> - width: var(--disclosure-button-size);
> - height: var(--disclosure-button-size);
> + width: 10px;
> + height: 10px;
Why are we making this smaller than the 15px it used to be? It feels like if this button fit, the new on should fit as well at the same height?
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:57
> + z-index: 1;
Why?
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:30
> + this._delegate = delegate;
I would add a `console.assert(delegate)` at the start of this constructor, since it appears to be required, and other things called delegates are sometimes optional in WI.
Honestly, I'm not sure delegate is the right word for this case. If the delegate were providing the popover's contents, it might make sense, but as it stands it's not really a delegate. Not sure what I would call it right this moment...
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:39
> + let bounds = WI.Rect.rectFromClientRect(this._delegate._nameElement.getBoundingClientRect());
If you want to access the nameElement, you need to make it public, as right now you are accessing a private member from a different class.
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:53
> + _searchDocumentationDetails(property)
NIT: `_getDocumentationDetails(property)`?
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:66
> + details.description = ContextualDocumentationDatabase[propertyName].description;
> + details.syntax = ContextualDocumentationDatabase[propertyName].syntax;
> + details.url = ContextualDocumentationDatabase[propertyName].url;
I think it's possible to throw an exception here if a valid property name made it here that wasn't in the database. I would change each of these to `ContextualDocumentationDatabase[propertyName]?.description` and so on so that if the property doesn't exist for some reason, you get back something nullish, instead of throwing an exception accessing a property of something that is potentially undefined. Alternatively, add a check before you `let details = {}` above that checks of the property exists in the database first, just in case. e.g.
```
if (!ContextualDocumentationDatabase[propertyName])
return {};
```
Additionally, if you believe it should be impossible to get to this point with a property name that isn't in the documentation, please add an assert above the previously suggested if to assert it. e.g. `console.assert(ContextualDocumentationDatabase[propertyName]`). We like to use asserts anytime we make this type of assumption, so that if we make code changes later that breaks the assumption, we realize it (hopefully) before a customer does.
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:80
> + let descriptionElement = documentationElement.appendChild(document.createElement("p"));
> + descriptionElement.textContent = details.description;
You should either handle properties without a description, or remove such properties from the database file. I'd opt for the second option.
> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:98
> + referenceLinkElement.href = details.url;
> + referenceLinkElement.textContent = "MDN Reference";
NIT: I'd reverse these two lines to better match the order you do the other elements in.
> Source/WebInspectorUI/UserInterface/Views/Main.css:345
> + width: 10px;
> + height: 10px;
ditto ComputedStylesDetailsPanel.js:54-55
> Source/WebInspectorUI/UserInterface/Views/Main.css:348
> + top: 1.5px;
I feel like Devin had a suggestion in his first review pass regarding vertical alignment without an explicit `top`. Did you try it?
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528
> + if (this._property.name in ContextualDocumentationDatabase || this._property.canonicalName in ContextualDocumentationDatabase) {
I actually had to look this one up; I wasn't familiar with the `in` keyword in this context. Can we change these to `ContextualDocumentationDatabase.hasOwnProperty(this._property.name)` and the sane for `canonicalName` to make sure we don't accidentally inherit something that looks like a CSS property from the Object prototype.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:529
> + this._contextualDocumentationButton.title = WI.UIString("Click to show documentation", "Tooltip to show documentation @ Contextual Documentation Popup");
See my note of strings near the top of this review...
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:538
> + this._contextualDocumentationButton.addEventListener("click", (event) => {
> + if (this._valueTextField?.editing) {
> + event.stopPropagation();
> + event.preventDefault();
> + this._valueTextField.discardCompletion();
> + }
> +
> + this._presentContextualDocumentation();
> + });
Not 100% sure on this, but I'm not immediately aware of other places where we inline event listeners like this. Instead, can you add a `_handleContextualDocumentationButtonClicked(event)` function to this class, and then here just do `this._contextualDocumentationButton.addEventListener("click", this._handleContextualDocumentationButtonClicked.bind(this));`
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:540
> + this._contextualDocumentationButton.title = WI.UIString("Documentation not available", "Tooltip for unavailable documentation @ Contextual Documentation Popup");
ditto :529
--
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/20210629/5aee4609/attachment-0001.htm>
More information about the webkit-unassigned
mailing list