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

[bugzilla-daemon at webkit.org]

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

--- Comment #17 from Harshil Ratnu <hratnu 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/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:1
>> +ContextualDocumentationDatabase = {
> 
> Are we planning to upload the script that creates this too?  If not, why not?

Yes we will upload the script to create this too, however in a different patch as that process is halted because our teammate who handles script is on vacation and my internship ends soon. So even though the script is ready it is prematures and needs to be integrated with the web inspector in the right place. Hence I will pass down the script to the team to integrate.

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

yes currently we only support -webkit- but maybe we should support these values too soon. All we need is a method to generate unprefixed version of these properties(maybe we already have and it is not in my knowledge) and hence I believe we should just keep them in the database. Do we prefer them removed from the database ?

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

I was unaware that such properties (without any details) existed. Thanks for pointing out. This has been fixed in the script and in the next patch.

>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:32
>> +.popover .documentation-popover > p {
> 
> Is the `.popover` needed?

Yes this was done to make this rule more specific than the " .popover p + p { margin-top :0.5 em } " rule

> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:1
> +/*

Done.

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

Fixed in next patch. Creating bounds in SpreadsheetStyleProperty.js now and passing it as an argument to present.

>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:53
>> +    _searchDocumentationDetails(property)
> 
> NIT: `_getDocumentationDetails(property)`?

yes that does sound better.

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

removed from database

-- 
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/20210630/e412d51c/attachment.htm>