Umami demo: Tour popup close button hover effect movement

Thanks for the issue and patch.

1. Checked the patch applies cleanly to Drupal 9.3, 9.4, and 10.
2. Needs testing so tagging.
3. Assume tests don't need to be added since this is a visual bug that would be difficult to create a test for.
4. CSS change is simple though not sure this is the best approach.
5. Issue summary could use the issue summary template so tagging.

Updated the steps to reproduce and linked to the screen recording.

hi @Kristen Pol,

Issue has been verified with version 9.4 and It works for me
Patch : https://www.drupal.org/files/issues/2021-12-25/3256002-2.patch
Screen recording added in the file location

Note : Patch file is not working in 9.3

Thanks for testing. You wrote:

Note : Patch file is not working in 9.3

but I think that was due to you using DrupalPod and/or simplytest.me and the tools weren't/wasn't working. I see that the Umami Demo profile doesn't work with simplytest.me right now and, since this issue is marked for 9.4, I don't think it can be tested with 9.3 on DrupalPod right now unless we switch the version on the issue.

But, I have found out from @catch that when the patch applies to more than one version, we can just test on 9.4 or 10, i.e. we don't have to test on all versions... just ones with different MRs/patches.

Note, in the future, if you add a new comment with new attachments, please explain why in the comment.

I've updated the issue summary.

Marking RTBC based on the above though I'm not sure the CSS approach is the best way to fix.

Unrelated failure. Back to RTBC.

+++ b/core/profiles/demo_umami/themes/umami/css/components/tour/tour.theme.css
@@ -7,6 +7,7 @@
+  border: 2px solid transparent;

I don't think adding a transparent border is an ideal solution for this because transparent borders can become visible with forced colors.

@lauriii Thoughts on a different approach? Add a margin?

Adding patch with negative margins on hover, which will allow the button to stay in place after adding border

Fixing lint issue with the above patch

Tagging for testing

Applied patch #16 cleanly on Drupal 9.5.x-dev and 10.1.x-dev.

The issue got fixed for 9.5.x-dev but it's still be seen on 10.1.x-dev

Refer attached videos for 9.5.x-dev.

Need to re-roll for 10.1.x-dev

Applied patch#16 on Drupal 9.5.x-dev it works fine now button is still their at place on hover too
Need to re-roll for 10.1.x-dev
Refer to the attached videos

Applied patch #21 on Drupal 10.1.x-dev.
Patch applied cleanly

The issue got resolved after applying the patch.
Not adding video as its same as #19

RTBC+1

Thanks everyone for great, thorough work here! I'm saving the issue credits, so taking some notes about who is credited and how, and advice for contributors.

1. Crediting @Gauravmahlawat for the original issue report, the original patch. and work on it since.
    

2. Creditng @lauriii for committer review.
    

3. Crediting @vulcanr and @ameymudras for coming up with a new implementation based on @lauriii's feedback.
    

4. Crediting @Satyajit1990 for the original video demonstrating the bug. I had to slow it to half speed and zoom in on the element, but I finally saw the "wiggle" in the before screenshots. Thanks for that manual testing! Next time, I would suggest cropping your video to only the affected portion of the screen so it's easier for reviews to understand and see.
   nbsp;

5. For @DeepaliJ: Thank you for reviewing this issue!

   Your before-and-after videos in #19 were helpful for me to understand the bug. Same advice as above; please crop them to only the affected portion of the screen. Thanks

   Also, regarding #22, the automated testing infrastructure tells us whether the change set still applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

   What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

   When you do post a review, be sure to describe what you reviewed and how with more specific details and steps. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

   Also see the issue credit guidelines for more information on which kinds of contributions are credited. In this case, you are still getting credit for the manual testing. 🙂
    

6. A reminder for @Kristen Pol to embed your UI changes screenshots in the issue summary. 🙂 Credited for manual testing and mentoring on the issue.
    

7. @gaurav-mathur, your comment just repeated automated testing that had been done by @DeepaliJ on the exact same patch, weeks after they did that work. That's a duplicate effort that adds noise to issues. So I am not crediting it. In the future, I suggest reading existing comments thoroughly and paying close attention to the next steps to receive credit.

I think this issue should have a final review from one of the frontend framework managers before it goes in to make sure it's the right approach, but I wanted to save time for them by taking care of the issue credit processing part of the review. 🙂

Hm I see, @Kristen Pol's screenshot was not of the correct UI element. Removing from summary. Sorry for the noise!

#21 adds an unwanted white border around the close button. A transparent border would probably work, though.

Would be useful to have before/after screenshots in the issue summary.

Thanks @Gauravvv!

Looks like I adviced against the transparent border in #11. As a second thought, it seems fine to have the transparent border appear in forced colors mode since it's a button.

Committed 2295e6e and pushed to 10.1.x. Cherry-picked to 10.0.x and 9.5.x. Thanks!