Refactor Claro's tablesort-indicator stylesheet

Changes done:
- Replaced physical properties (left, right, etc.) with their logical equivalents (inline-start, inline-end, etc.)
- Combined multiple tablesort classes into one

If you have any feedback or questions, please let me know.

I would also like to contribute further by solving additional issues to align with the latest coding standards, if this works.

Please review the latest commit with the original file.

Gauravvv → made their first commit to this issue’s fork.

Fixed CCF in #6, and refactored the code.

Since this changes the .css file can we please get some before/after screenshots.

Thanks!

Added before and after patch screenshots. Please review
Before patch:

After patch:

Thanks @Gauravvv

Left a comment on the MR. This should be manually tested on RTL too since there's some CSS specific to RTL.

Addressed feedback over MR.
Attached after patch screenshot for RTL. Please review

Thanks!

The specificity of the selector is changed, there is no need to prefix the rules with .tablesort

Removed unused . tablesort class.

another question in the MR

Seems background-color was changed back so should be good now.

Need someone to test on windows in high contrast mode with a LTR and a RTL language to make sure the changes here work as intended.

With screenshots too

Need someone to test on windows in high contrast mode with a LTR and a RTL language to make sure the changes here work as intended.

Good catch asking to check this. Because of the specificity of the [dir="rtl], the background image is showing above the mask-image in forced colors mode, making the arrow look kinda weird (as they're superimposed).

I'll push some code shortly.

Pushed a fix by flipping the .tablesort element. This also has the benefit of no longer needing the RTL version of the icon.

Uploading some screenshots

Everything looks good to me.

The MR is currently blocked and needs to be rebased. Also, the version on the issue is 11.x-dev but the target branch on the MR is 10.1.x, so maybe the target branch on the MR needs to be updated as well? Just a bit of git-related cleanup required before this can be reviewed by a committer. Thank you!

I have added patch for 11.x. Also I have made changes in core/themes/claro/css/components/tables.pcss.css file, as we have renamed the sort-inactive icon. I have updated the name here as well. please review

@Gauravvvv Thanks for your attention to the “Needs reroll” tag, but this issue appears to be using an MR-based workflow, not a patch workflow. I realize that the “Needs reroll” tag could be confusing, when what is needed is a rebase on the merge request, not a patch file. Please refer to the docs on rebasing a merge request: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

Also, it does look like there are a number of unresolved threads in the MR that should be marked as resolved if the feedback has been addressed.

If the issue can be updated via the MR.

Need to change the target branch, Author can change it to 11.x

As the rebase on 3332701-refactor-claros-tablesort-indicator is broken i did a new one on rebase-testing (sorry for the poor name). Someone who actually worked on the code should review the result. I run the build:css script after every during every merge conflict and after the final one a lint:css

MR should be updated to point to 11.x or hidden and a new MR for 11.x be placed.

shmy → changed the visibility of the branch rebase-testing to hidden.

shmy → changed the visibility of the branch 3332701-refactor-claros-tablesort-indicator to hidden.

Merge request pipeline failed.

Here are screenshots for renamed sort--inactive.svg image tables.pcss.css file:

Before (default):

After (default):

Before (forced-colors):

After (forced-colors):

The :is() selector doesn't work for me in neither Firefox or Chromium. I haven't worked with PostCSS before and i don't get why its added there. The following screenshots are made w/o the :is() selector as there is no difference for me when set. I don't know if this is a different issue or i'm making something wrong.

Before (RTL default):

After (RTL default):

Before (RTL forced-colors):

After (RTL forced-colors):

I removed the @nest deprecated directive and solved the linting errors. Adding before and after screenshots.
RTL-Before

RTL-After

LTR-Before

LTR-After

This looks correct based on the numerous screenshots.

Committed 9e0ed31 and pushed to 11.x. Thanks!

nod_ → closed merge request !5864

Automatically closed - issue fixed for 2 weeks with no activity.