[webkit-reviews] review denied: [Bug 200212] Web Inspector: Resources: Display outline around images when viewing image collections : [Attachment 375055] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 200212: Web Inspector: Resources: Display outline around images when
viewing image collections
https://bugs.webkit.org/show_bug.cgi?id=200212

Attachment 375055: Patch

https://bugs.webkit.org/attachment.cgi?id=375055&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 375055
  --> https://bugs.webkit.org/attachment.cgi?id=375055
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375055&action=review

r-

I like the idea, but I don't think we should only show a "highlight"/"callout"
on `:hover`, as that requires user interaction (see comment below)

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:37
> +    outline: 1px solid var(--text-color-quaternary);

This is an odd use of this variable.  The outline of an <img> has nothing to do
with text colors.  For these cases, I think we either should generalize our
variable names ("--text-color-quaternary" to just "--color-quaternary"), or not
even use a variable (which is fine, given that this a specific usage).

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:38
> +    cursor: pointer;

We typically only use `cursor: pointer;` for actual links, not clickable
things.  Please remove.

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:41
> +.content-view.collection .resource.image img:hover {

"limiting" this to only on `:hover` doesn't actually solve the issue described
in this bug.  All it does it make it clearer during an interaction, but not at
a glance.  I'd either expect us to _always_ add an
`outline`/`border`/`box-shadow` or do some sort of other always-visible
styling.