Add hover state to site thumbnail #357
Conversation
| { ! loading && siteRunning && ( | ||
| <button | ||
| aria-label={ __( 'Open WP Admin' ) } | ||
| className={ cx( `relative group` ) } |
There was a problem hiding this comment.
I couldn't make the focus state work and it was yellow all the time. How to do it correctly?
There was a problem hiding this comment.
Did you try using something like Button component that we have: https://github.com/Automattic/studio/blob/trunk/src/components/button.tsx ? It would take care of all these styles including focus so you would not need to manually fix them
There was a problem hiding this comment.
Yes, and it was even worse. I've just pushed the fix for the outline issue.
There was a problem hiding this comment.
Hahaha, it does not surprise me 😅
Ideally I would love for us to use the components we have instead of adding the button manually but I have not explored this solution in detail.
| `opacity-0 group-hover:opacity-90 group-hover:bg-white duration-300 absolute size-full flex justify-center items-center bg-white text-a8c-blueberry` | ||
| ) } | ||
| > | ||
| { __( 'Open Site' ) } |
There was a problem hiding this comment.
| { __( 'Open Site' ) } | |
| { __( 'Open site' ) } |
This seems to be the spelling according to the design
There was a problem hiding this comment.
I fixed the letter casing, thanks.
| { ! loading && ( | ||
| { ! loading && siteRunning && ( | ||
| <button | ||
| aria-label={ __( 'Open WP Admin' ) } |
There was a problem hiding this comment.
I am finding a bit strange that the aria-label is Open WP admin but the text on the actual thumbnail is Open site. Should we consolidate the two to be consistent there? Perhaps the text on the thumbnail should be Open WP admin if that is what the action of clicking on the thumbnail does?
There was a problem hiding this comment.
I fixed it already when I changed the link to open site frontend instead of WP Admin.
|
|
||
| return ( | ||
| <div className="p-8 flex max-w-3xl"> | ||
| <div className="w-52 ltr:mr-8 rtl:ml-8 flex-col justify-start items-start gap-8"> | ||
| <h2 className="mb-3 a8c-subtitle-small">{ __( 'Theme' ) }</h2> | ||
| <div | ||
| className={ cx( | ||
| 'w-full min-h-40 max-h-60 overflow-hidden rounded-sm border border-a8c-gray-5 bg-a8c-gray-0 mb-2 flex items-center justify-center', | ||
| 'w-full min-h-40 max-h-64 rounded-sm border border-a8c-gray-5 bg-a8c-gray-0 mb-2 flex justify-center', |
There was a problem hiding this comment.
I'm trying to fix the missing top and bottom thumbnail outline on focus.
There was a problem hiding this comment.
Carlos' suggestions make sense.
I tested the Pr and it looks good. I have two questions:
- I think the hover effect should be present even if the server is stopped. In that case we should start the server before opening the browser.
- The focus effect should be the same as the hover effect, displaying the open site effect, as shown here Feature Request: Clicking site thumbnail should open site #103 (comment)
Screencast:
U9djHa.mp4
|
Thanks for testing it!
It may make some sense. However, it would introduce a difference between the 'WP Admin' and 'Open site' buttons and the thumbnail. The buttons in the site header are disabled when the site is not running, and they don't allow the site to start automatically. For those reasons, I would prefer to keep those aligned. What if we displayed the disabled cursor over the thumbnail on a stopped site?
Thanks, fixed: 8242879 |
Related to #103
Proposed Changes
I propose adding a hover state to the site thumbnail to let the user open the site by clicking it.
Testing Instructions
Pre-merge Checklist