Hover effect is shown on the full row instead of a single box

This behavior is the same as in Seven so this is not a regression. This is the standard behavior of tables. We might want to consider changing this in the future if we think it would improve the UX.

CSS improved. ready for review.

I applied patch #5, It's working for me.
I am attaching a screenshot before and after applying the patch. Thank you!

1. +++ b/core/themes/claro/css/components/tables.css
   @@ -333,3 +333,12 @@ td.priority-medium {
   +/* hover improve. #3101460 */
   

   We don't add comments like this with issue numbers in.

2. +++ b/core/themes/claro/css/components/tables.css
   @@ -333,3 +333,12 @@ td.priority-medium {
   +.image-anchor tr:hover, .image-anchor tr:focus {
   +  background: none;
   +}
   +
   +.image-anchor tr td:hover, .image-anchor tr td:focus {
   +  background: #f0f5fd;
   +}
   

   Claro's css is created from .pcss files - these would need to be updated and this should be the compiled outcome - if this is what we desire.

Tagging for frontend framework manager review since #4 is ambiguous about whether we should make this change. Thanks!

I'm not 100% sure if the new hover effect is correct either. Having hover effect on the cell could be confused with the hover effect on the option element. I'm wondering if we should either make this work so that there's no hover effect on the table cell at all, or so that the table cell can be clicked for selecting the option.

#14

If there's no hover effect on the table cell.

Review the patch also attached the screen shot for the reference if there
is no hover effect on the table cell.

+++ b/core/themes/claro/css/components/tables.pcss.css
@@ -141,7 +141,6 @@ thead tr {
 tr:hover,
 tr:focus {
   color: var(--color-text);
-  background: var(--color-bgblue-hover);
 }

This hover styling should not be removed completely. For example, this hover effect is used to good effect on table rows in admin/content and this should not be removed. This change should be more targeted so the hover still exists where it is wanted.

Hello,

Kindly review the attached patch.

Option 1:
As per comment #14 no hover effect on the table cell at all & #16 onhover of table td removed background color for targeted element only.

Thanks!

Hello,

Kindly review the attached patch.

Option 2:
As per comment #14 table cell can be clicked for selecting the option with hover background color.

Thanks!

Forget to add interdiff(interdiff_10-20), hence adding here.

Updated patch for #20 work with interdiff.

Thanks!

It's not yet clear if this should be targeting Drupal core instead of just Claro, since the reported symptom is not unique to Claro.

I still want to share feedback as it's the kind of information that will make contributing to Claro easier. I'm not 100% it will get this closer to completion since it may be best to convert this to a core issue.

1. +++ b/core/themes/claro/claro.info.yml
   @@ -22,6 +22,7 @@ libraries:
     - core/drupal.message
     - core/normalize
     - claro/global-styling
   + - claro/image-anchor
   

   There's no reason to include this library on every page. It could just extend the image/admin library, which would mean it's only loaded where needed.

2. +++ b/core/themes/claro/claro.theme
   @@ -21,6 +21,15 @@
   +/**
   + * Implements hook_theme_suggestions_HOOK_alter() for form templates.
   + */
   +function claro_theme_suggestions_form_alter(array &$suggestions, array $variables) {
   +  if ($variables['element']['#form_id'] == 'image_effect_form') {
   +    $suggestions[] = 'form__image_effect_form';
   +  }
   +}
   +
   
   +++ b/core/themes/claro/templates/form/form--image-effect-form.html.twig
   @@ -0,0 +1,18 @@
   +{#
   +/**
   + * @file
   + * Default theme implementation for a 'form' element.
   + *
   + * Available variables
   + * - attributes: A list of HTML attributes for the wrapper element.
   + * - children: The child elements of the form.
   + *
   + * @see template_preprocess_form()
   + *
   + * @ingroup themeable
   + */
   +#}
   +{{ attach_library('claro/image-anchor') }}
   +<form{{ attributes }}>
   +  {{ children }}
   +</form>
   

   If you are using a theme hook to suggest a new template, and the only difference is adding a library, unless I"m missing something it's going to be more efficient to add the library in a form_alter hook and eliminate the need for an additional template.

   Although the template probably shouldn't be used at all, I want to point out that the @ingroup themeable and the "Default theme implementation " in the opening comments are not accurate if the template is in a theme. Templates in themes have no @ingroup and this isn't a "Default theme implementation for a 'form' element.". Reference other templates in themes to see how this is typically formatted.

3. 
   diff --git a/core/themes/claro/js/image-anchor.js b/core/themes/claro/js/image-anchor.js
   new file mode 100644
   
   +++ b/core/themes/claro/js/image-anchor.js
   +++ b/core/themes/claro/js/image-anchor.js
   @@ -0,0 +1,17 @@
   

   Drupal JS should be added via .es6.js files, which are then compiled into .js files.This would also not pass JS code standards, which scan the .es6.js files. More info here: #2815083: Drupal core now using ES6 for JavaScript development

Thanks, @bnjmnm for option 2 patch review. Got to know good things from your comment and which will make contributing claro easier for me onwards.

Kindly review the attached updated patch for option 2(table cell can be clicked for selecting the option with the hover background color).
option 1 is #19

Two options provided whichever is finalized we can use that patch.

Thanks!

Option 1 :- Tested and Verified by applying patch #19. And it's working as expected. As part of this patch the hover effect is removed from the tiles and the user is only able to select by clicking on radio button.

Option 2 :- Tested and Verified by applying patch #25. As part of this patch, the hover effect is only for the tiles. We are able to select the options by clicking any where inside the tiles.

#25 (hover-effect-3101460-25.patch) is showing 24 coding standards violation message, we need to address those, isn't it?

Discussed this on the usability call earlier this week. They liked the most recent approach where the radio button can be activated by clicking the table cell because it makes it easier to click the input. There's still two different hover effects for when the table cell is being hovered and when the radio button is being hovered. Since there's no difference in clicking the table cell and the radio button, we should unify the hover effect for the two different states.

Thanks, @lauriii for finalizing the approach to go ahead.

Kindly review the attached patch. As per #35 removing two hover(table cell & radio) and keeping only one hover on table cell. See the below GIF

Thanks!

@ambuj_gupta are u working on this issue

No @samiullah, i am not working on this issue. I just forgot to unassigned.

Patch #37 is applied successfully.

Testing Steps Followed:
- Go to Configuration -> Image Style -> Click edit in front of Large -> Add Scale and Crop effect from dropdown
- See the hover changes

Looks good to me.
Can be a move to RTBC
Please refer attached screenshots.

Thanks everyone for moving this issue along.

The patch in #37 has coding standard errors, see https://www.drupal.org/pift-ci-job/1781667

It is good to see that the issue summary has before and after screenshots. However, there are 4 after screenshot with no explanation of their context. The issue summary should also include the proposed resolution and any remaining tasks. I suggest using dreditor to add issue summary template and then to complete that. Having a complete and up to date Issue Summary helps the committers!

Setting NW for coding standards, issue summary update and I don't see that a code review has been done on the latest patch.

The CSS linting errors on DrupalCI are not relevant because Drupal has moved from using CSSLint into using Stylelint. Agreed that the issue summary could use an update. I think it would be good idea to also discuss this in one of the UX calls before committing.

Fixed coding standard issues

Patch applied working correctly sharing ref...

Thanks .......

Verified and tested patch #50.
Patch applied successfully and looks good to me.

Testing Steps:
# Go to Configuration -> Image Style -> Click edit in front of Large -> Add Scale and Crop effect from dropdown
# See the hover changes

Test Results:
# The user is able to see the hover effect while creating image style but the square cell is not clickable. Earlier it's working but in Patch #50 It's not working.

Please refer attached screenshots.
Looks good to me.
Can be a move to +1RTBC.

Checked and patch in #50 still applies cleanly to Drupal 9.3 and 9.4 and applies with offsets to Drupal 10 (see below).

Styling was updated per #46 in latest patch but these have not been addressed from that comment:

Agreed that the issue summary could use an update. I think it would be good idea to also discuss this in one of the UX calls before committing.

so still needs:

• Maintainers to discuss in UX meeting
• Issue summary update
• If approach is approved, still needs testing on Drupal 9.4 and 10

[drupal-10.0.x-dev/10.0.x] [drupal-10.0.x-dev]$ patch -p1 < 3101460-50.patch 
patching file core/themes/claro/claro.info.yml
patching file core/themes/claro/claro.libraries.yml
Hunk #1 succeeded at 304 (offset 1 line).
patching file core/themes/claro/css/components/tables.css
patching file core/themes/claro/css/components/tables.pcss.css
Hunk #1 succeeded at 241 (offset -2 lines).
patching file core/themes/claro/js/image-anchor.es6.js
patching file core/themes/claro/js/image-anchor.js

Hiding screenshots so it's easier to see patches.

This a good improvement for the UX, thanks! But I think there's still one small thing missing on hover: I'd expect to see the cursor to convert to pointer when hover to show that it is an interactive element, as with the default radio element.

Included a patch which would change the pointer on hover

Reviewed and tested patch #60 for 9.5.x. Adding Screenshots for the reference. Need +1 RTBC

Reviewed and tested patch #60 for 9.5.x.
Adding Screenshots for the reference.
Thanks ameymudras.

Verified and tested patch #60 on Drupal 9.5.x-dev. Patch applied successfully and looks good to me.

Testing steps:
1. Go to admin/Appearance
2. Set Claro theme in the Administration theme (Scroll down and see)
3. Save configuration
4. Go to Configuration > Image Style > Click edit in front of Large > Add Scale and Crop effect from dropdown > Add
5. See the hover effect
6. Apply the patch
7. Reload the page. (Note: Clear all caches if required)
8. Now observe the hover effect

Testing Result:
1. After applying the patch the hover effect is changed

Refer to screenshots and videos for reference
Can be moved to RTBC

Removing credit for duplicate screenshots on same patch, per How is credit granted for Drupal core issues.

1. +++ b/core/themes/claro/css/components/tables.pcss.css
   @@ -254,3 +254,40 @@ td.priority-medium {
   +}
   ...
   +}
   

   This is quite a bit of styling for a very specific widget. To fix the ui bug all that is needed is

   	+.image-anchor tr:hover,
   +.image-anchor tr:focus {
   +  background: none;
   +}

   and I'd likely commit that (and I'm a FEFM so I could commit it...)

   This is introducing a new UI pattern that makes non-interactive elements behave interactively. This would require looking into the accessibility implications.

   If there's a feeling it will truly benefit Drupal it could be explored in a followup issue, but lets just disable the row hover change. The element that should change on hover (the radio input), does change on hover.

2. +++ b/core/themes/claro/js/image-anchor.es6.js
   @@ -0,0 +1,17 @@
   +        .once('imageAnchor')
   

   jQuery once is deprecated in Drupal. Use the drupal/once library instead.

Thanks @bnjmnm as per your suggestion I have removed the extra styling from the table.pcss.css and updated the js to make use of drupal/once

Thanks for updating. Tagging for manual testing the new patch.

Patch #67 applied successfully on Drupal 9.5.x-dev.

The hover effect is displaying correctly after applying the patch
Before patch:

After patch:

But, the patch failed to apply on Drupal 10.1.x-dev as its giving an error as shown below:

Checking patch core/themes/claro/claro.info.yml...
error: while searching for:
    - claro/media_library.theme
  media_library/widget:
    - claro/media_library.theme

quickedit_stylesheets:
  - css/components/quickedit.css

error: patch failed: core/themes/claro/claro.info.yml:164
error: core/themes/claro/claro.info.yml: patch does not apply
Checking patch core/themes/claro/claro.libraries.yml...
Hunk #1 succeeded at 287 (offset -19 lines).
Checking patch core/themes/claro/css/components/tables.css...
Hunk #1 succeeded at 282 (offset -27 lines).
Checking patch core/themes/claro/css/components/tables.pcss.css...
Hunk #1 succeeded at 231 (offset -23 lines).
Checking patch core/themes/claro/js/image-anchor.es6.js...
Checking patch core/themes/claro/js/image-anchor.js...

Needs to be re-roll for Drupal 10.1.x-dev

#71 Please review the patch for 10.1.x-dev

try to fix #71

Applied patch #72 on Drupal 10.1.x-dev version
The patch applied cleanly.

The issue has been fixed. The hover effect is displaying correctly after applying the patch.
Screenshots are as per the #70

RTBC +1

Patch #60 applied successfully on Drupal 9.5.x-dev and looks good to me.
After applying the patch the hover effect is changed.
Refer to screenshots.

@gaurav-mathur Please look at previous comments before testing. 1) The latest patch is in #72, 2) It was already tested in #73 so you are duplicating effort, and 3) you shouldn't assign Drupal core issues to yourself. Thanks.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Reroll #72 for 10.1.x

+++ b/core/themes/claro/js/image-anchor.es6.js
@@ -0,0 +1,18 @@
+  Drupal.behaviors.imageAnchor = {

+++ b/core/themes/claro/js/image-anchor.js
@@ -0,0 +1,19 @@
+ * DO NOT EDIT THIS FILE.

es6 files no longer needed for 10.x https://www.drupal.org/node/3305487

Removed image-anchor.es6.js file. Attached interdiff for same. Please review

Was tagged for issue summary update in #45 which still needs to happen from what I can tell.

Did not review.

patch is created for hovering on the anchor instead of on the particular box a hover effect plz review

Re #85 it was already established that a solution applied to the entire table would cause regressions in tables such as the one in admin/content. It can be annoying to read through all the comments in a long issue, but taking that time to read save you from wasting time on an approach that has already been rejected.

HINT: #66 mentions a way to address this that will not cause regressions and is something that I'd be willing to commit, so I'd encourage someone to take advantage of that answer key being hidden in plain sight.

Looking at this one at DrupalCon 2023

Updated summary with steps to reproduce

Added this in /workspace/DrupalPod/web/core/themes/claro/css/components/tables.pcss.css

.image-anchor tr :not(:hover) {
background-color: #ffffff;
}
.image-anchor tr :hover > div {
background-color: transparent;
}

It works! :D

This patch applies to Drupal 9.5.x
Needs testing on Drupal 11.x

Applied the patch cleanly to core 11.x c8806a02991f04233f859d2ef8870363172a2186

@trackleft2 This is definitely not ready for RTBC. Applying cleanly is far from the only criteria taken into account when making this decision. A patch with unwanted changes can apply cleanly! In addition, every patch uploaded to a Drupal issue has a suite of automated tests run. All of these tests must pass for an issue to be committable.

In this case, it is getting a "custom commands failed" error. That means the patch is not even passing general code formatting tests. Clicking that message will take you to a page that lists the specific reasons those checks failed and point the contributor towards what must be changed to get it working.

New patch and interdiff

Cleaned up the tags some.

Did remove the submaintainer review tag as not sure that's needed. This is about fixing a hover effect and not how image style functions.

Following the issue summary steps I was able to confirm the issue on Drupal 11.x with a standard profile install.
Applying patch #97 when hovering over individual squares those squares are highlighted vs the whole row.
Saved the image style, edited by moving location and everything is working as expected.

Spotted some additional things that are likely easy to address.

1. +++ b/core/themes/claro/css/components/tables.css
   --- a/core/themes/claro/css/components/tables.pcss.css
   +++ b/core/themes/claro/css/components/tables.pcss.css
   

   Although the changes work when added here, it's better if they are added to image.admin.pcss.css as that's where the other .image-anchor styles are, including ones for .image-anchor <tr>, so it will keep related styles nearby and reduce repetition.

2. +++ b/core/themes/claro/css/components/tables.pcss.css
   @@ -141,6 +141,12 @@ tr.color-error:focus {
   +.image-anchor tr :hover>div {
   

   There should be a space on each side of the >. If this was the only thing I'd just fix it on commit - not worth un-RTBCing for just that.

OK, I've renamed image.admin.css to image.admin.pcss.css and per the recommendation.

Removed the styles from core/themes/claro/css/components/tables.pcss.css
Added the styles to modules/image/css/image.admin.pcss.css

OK, I've renamed image.admin.css to image.admin.pcss.css and per the recommendation.

Removed the styles from core/themes/claro/css/components/tables.pcss.css
Added the styles to modules/image/css/image.admin.pcss.css

There actually isn't a image.admin.pcss.css file in the images module.

But changes have been moved and point 2 in #100 appear to have been addressed. Remarking.

Thank you for the help @smustgrave and @trackleft2!!

Tried to address #100 on top of #97.

Still seems to be working.

Random failure

It's been tough, but after 3.5 years, 108 comments, and enough screenshots to feed a small nation, the combined efforts of 37 contributors have resulted in the image styles page getting this cool slice of CSS.

+.image-anchor td:not(:hover) {
+  background-color: var(--color-white);
+}
+
+.image-anchor td:hover > div {
+  background-color: transparent;
+}

And tbh I almost dug in to see if the CSS could get a little more optimized but you know what? This works, it's an infrequently visited area of a site, not likely to result in conflicts or scalability issues. Enjoy the nicer hover in that snazzy grid!