WordPress / wporg-theme-directory

12 stars 6 forks source link

Hover over theme patterns displays the default cursor instead of a pointer (hand) and there's no outline #128

Closed ndiego closed 3 weeks ago

ndiego commented 4 weeks ago

Basically, the hover states for patterns on single theme pages are not the same as the hover states for style variations, and I think they should be the same. Patterns are missing the highlighted border and the pointer cursor style. Unfortunately the screen recording tool I'm using is not picking up the different cursor styles, but you can test by looking at the Twenty Twenty-Four page.

We also might want some transition applied to the animation, but I leave that to @WordPress/meta-design

https://github.com/WordPress/wporg-theme-directory/assets/4832319/df9d0f15-a90b-41e8-8649-20b813436297

ndiego commented 4 weeks ago

Related to #126

ryelle commented 4 weeks ago

We also might want some transition applied to the animation, but I leave that to @WordPress/meta-design

Ah, I was going to do that.

Patterns are missing the highlighted border and the pointer cursor style.

Good catch on the cursor 👍🏻 The styles for this were only designed for the previewer, so I kind of … backported them to something that makes sense on this page. I agree the border change should be the same though, and since it was just updated in plugins, I'll go ahead and have the hover darken border color on patterns too.

StevenDufresne commented 4 weeks ago

The pattern border also appears to be thicker than the screenshot and style variations border.

Screenshot 2024-06-18 at 10 23 18 AM
jasmussen commented 4 weeks ago

Good issue. A few things here, let's add a transition on both. Something a la transition: all 0.2s ease; does this:

ease

That's a choppy GIF but hopefully the animation comes across. For reduced motion we have this mixin in the block editor:

@mixin reduce-motion($property: "") {

    @if $property == "transition" {
        @media (prefers-reduced-motion: reduce) {
            transition-duration: 0s;
            transition-delay: 0s;
        }
    } @else if $property == "animation" {
        @media (prefers-reduced-motion: reduce) {
            animation-duration: 1ms;
            animation-delay: 0s;
        }
    } @else {
        @media (prefers-reduced-motion: reduce) {
            transition-duration: 0s;
            transition-delay: 0s;
            animation-duration: 1ms;
            animation-delay: 0s;
        }
    }
}

Steve is right about the border width, there's also something off about the radius:

Screenshot 2024-06-18 at 08 41 26

It's because there's an abs-positioned element on top of a pattern preview, and the outer radius is 4px, the inner radius is 3, but it doesn't fully add up. I wonder if the border could be set on .wporg-theme-listbox .wp-block-wporg-screenshot-preview instead? It should be 4px radius, 1px width, 1.5px only on focus. Can have the same easing.

ryelle commented 4 weeks ago

Moving the issue from #139 to a comment here, it doesn't need to be a separate issue.

This is in nit-picky territory, and it's probably related to https://github.com/WordPress/wporg-theme-directory/issues/128#issuecomment-2175270424 and how hover/focus styles are drawn on thumbnail previews.

But in case a refactor allows us to improve things, there are small edge artifacts on thumbnails. It's not immediately visible:

Image

But if you zoom in, you can see the white artifacts:

Image

This is likely from the focus style being stacked on top of a white border, using a pseudo element, rather than being drawn on the parent instead.

ryelle commented 3 weeks ago

Fixed:

The white artifact is due to the white background, which is necessary for the patterns, otherwise they look like this:

Screenshot 2024-06-18 at 4 16 27 PM

And it needs to be a pseudo-element so that I can nest "borders" & have an inset border. If it's applied to the parent, it's hidden behind the image (plus the change in border width would impact the container size, so everything would shift). I know it's strange, but I did do that for a reason.

Let me know if I missed anything.

jasmussen commented 3 weeks ago

And it needs to be a pseudo-element so that I can nest "borders" & have an inset border. If it's applied to the parent, it's hidden behind the image (plus the change in border width would impact the container size, so everything would shift). I know it's strange, but I did do that for a reason.

I'll take your word for it.

In some other contexts, we've used inset shadows to accomplish the same. If that gets paired with something like a outline: 2px solid transparent;, we account for Windows high contrast mode as well, which removes shadows, and forcibly changes any opacities to full-opacity.