Created attachment 375053 [details] [Image] Before I love how we display all images when selecting "Images" folder in the navigation sidebar. Many websites use image sprites. google.com is one of those websites. When selecting "Images" folder, it's hard to guess how many image resources I'm looking at.
Created attachment 375054 [details] [Image] After
Created attachment 375055 [details] Patch
Comment on attachment 375055 [details] 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.
(In reply to Devin Rousso from comment #3) > Comment on attachment 375055 [details] > 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) Based on the Before/After images, it looks like there was a gray border when not-interacting. That actually looks really good! Did that alone not satisfy your concern?
(In reply to Joseph Pecoraro from comment #4) > Based on the Before/After images, it looks like there was a gray border when not-interacting. Oh I totally missed that! Perhaps we should increase the contrast so it's more visible (given that I didn't notice it before)? > That actually looks really good! Did that alone not satisfy your concern? I'm not a fan of the color changing when hovering, so I'd still like that removed. While we're at it, I think it would be awesome to add a `WI.NavigationItem` for toggling the image transparent background style (the checkerboard icon). We already show it for individual image resources (and canvas rendering contexts).
Comment on attachment 375055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375055&action=review >> 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). --text-color-quaternary is only used as a border/outline color. This is confusing. /* Dividers, separators, background fills */ --text-color-quaternary: hsl(0, 0%, 85%); This was actually a part of Dark Mode guidelines in 2018, but this doesn't seem to be the case anymore! Quaternary label color The text of a label of lesser importance than a tertiary label such as watermark text. https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors Perhaps I should add this variable: Separator color A separator between different sections of content. >> 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. The light outline (line 37) is always visible, even without hover. I'd be fine with a light box-shadow as well, but I don't see a lot of shadows in the latest macOS releases.
(In reply to Devin Rousso from comment #5) > (In reply to Joseph Pecoraro from comment #4) > > Based on the Before/After images, it looks like there was a gray border when not-interacting. > Oh I totally missed that! Perhaps we should increase the contrast so it's > more visible (given that I didn't notice it before)? Probably quaternary is a bit dim. Navigation items normally have a currentColor of: color: hsl(0, 0%, 45%); And on active: color: hsl(0, 0%, 30%); Maybe those would work better. > I'm not a fan of the color changing when hovering, so I'd still like that > removed. I like the hover. It would be like our :normal / :hover / :active colors highlights on other items. I could see clicking the image taking directly to that Image Resource, which would be pretty nice! > While we're at it, I think it would be awesome to add a `WI.NavigationItem` > for toggling the image transparent background style (the checkerboard icon). > We already show it for individual image resources (and canvas rendering > contexts). This sounds like a good idea. Could be done separately.
> I like the hover. It would be like our :normal / :hover / :active colors > highlights on other items. I could see clicking the image taking directly to > that Image Resource, which would be pretty nice! I say this having not used it yet myself. It just seems like it might focus more when mousing over, but I don't know if it would be distracting in normal use or not.
(In reply to Joseph Pecoraro from comment #7) > I like the hover. It would be like our :normal / :hover / :active colors > highlights on other items. I could see clicking the image taking directly to > that Image Resource, which would be pretty nice! We already do this! I thought changing cursor and adding border hightligh would make it more obvious.
Created attachment 375121 [details] Patch I changed the outline to be darker (in the light mode; lighter in the dark mode) and removed the hand cursor. I kept the accent color outline on hover.
Created attachment 375122 [details] [Image] After
Comment on attachment 375121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375121&action=review > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:37 > + outline: 1px solid var(--border-color); What does this look like with a 0.5px outline-width instead (or with a lighter color, like maybe a `--border-color-light`)? I'm concerned that it's too similar to the various borders around the `WI.ContentView`. Ideally, this border should identify the bounds of each image, but not so much so that its confusing as to where the actual content area begins/ends. > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:41 > + outline-color: var(--glyph-color-active) It's unfortunate that the only CSS variable we have for the system accent color is "limited" to glyphs :( A more generic name would've been nice, like `--target-color-active` or `--active-color-active`.
Comment on attachment 375121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375121&action=review >> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:37 >> + outline: 1px solid var(--border-color); > > What does this look like with a 0.5px outline-width instead (or with a lighter color, like maybe a `--border-color-light`)? I'm concerned that it's too similar to the various borders around the `WI.ContentView`. Ideally, this border should identify the bounds of each image, but not so much so that its confusing as to where the actual content area begins/ends. Oh also, why not use a `border`? I realize that a `border` would affect the sizing of the element, but I think that's what we want in this case (everything should already have extra padding, so there's no worry of an overlap).
Created attachment 375175 [details] Patch I like the 0.5px wide outline.
Created attachment 375176 [details] [Image] After
Comment on attachment 375175 [details] Patch r=me, thanks for iterating! looks awesome :)
Comment on attachment 375175 [details] Patch Clearing flags on attachment: 375175 Committed r248019: <https://trac.webkit.org/changeset/248019>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53730622>