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.
| <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.
fluiddot
left a comment
fluiddot
left a comment
There was a problem hiding this comment.
LGMT 🎊 ! I've added some comments but none should be considered a blocker for this PR.
sejas
left a comment
sejas
left a comment
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 #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 |
fluiddot
left a comment
fluiddot
left a comment
There was a problem hiding this comment.
Re-tested using the latest changes. It worked as expected 🎊.
sejas
left a comment
sejas
left a comment
There was a problem hiding this comment.
Thanks for fixing the group-focus 👍
4 participants
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