Problem/Motivation

See #3432183: Move system/base component CSS to respective libraries where they exist.

tablesort.module.css is in system/base but it could probably be in its own library, then attached when building tablesort headers.

Steps to reproduce

Go to admin/content and check that the sticky tableheader is styled correctly including after clicking on a sortable column.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#27 Screenshot 2025-04-19 at 11.46.37 AM.png54.81 KBlauriii
#21 3432244-nr-bot.txt91 bytesneeds-review-queue-bot
#19 3432244-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3432244

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

kentr’s picture

kentr commented
Related issues: +#2880237: [meta] Refactor system/base library

Possible duplicate of #2880237: [meta] Refactor system/base library, as some work for tablesort has already been done in that issue.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Closed (duplicate)

Good point - I think I opened this before I found that one. Closing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Closed (duplicate) » Active

Actually no. We should split the change out to here since it's tricky.

xamount made their first commit to this issue’s fork.

xamount’s picture

xamount
Primary language English
commented
Status: Active » Needs work

I've adjusted the tablesort.module.css but I could not figure where is the best place to attach the new library.

Someone please review and guide me here.

catch opened merge request !11445

catch changed the visibility of the branch 3432244-split-tablesort.module.css-out to hidden.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

@xamount this is a tricky one, I ended up opening a new MR because there's been some related changes in core since your branch was open. With attachments, I wasn't sure either but added a template_preprocess implementation which works and is at least compact.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs manual testing

Should be reviewable/testable now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

So the way I tested this one was by just created a few nodes with A,B,C in front of the title. Then on the content view verified the table sorting worked as expected.

If different types of sorting are needed let me know, else believe this is good.

nod_ made their first commit to this issue’s fork.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

Paths needs to be updated after css file move

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Updated the icon paths, good spot, not sure how I missed it in manual testing, maybe claro overrides hid the bug?

nod_’s picture

nod_
Primary language French
Location Lille
commented

Yes I don't think any core theme uses this file

smustgrave’s picture

smustgrave commented
Status: Needs review » Fixed

Feedback appears to be addressed.

smustgrave’s picture

smustgrave commented
Status: Fixed » Reviewed & tested by the community

Oops my mistake.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3432244-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3432244-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

dcam made their first commit to this issue’s fork.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Rebased

nod_ closed merge request !11445

  • nod_ committed 41663286 on 11.x 
    Issue #3432244 by catch, dcam, xamount, nod_, smustgrave: Split...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 4166328 and pushed to 11.x. Thanks!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new Screenshot 2025-04-19 at 11.46.37 AM.png54.81 KB

There's a regression with this change:

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Fixed » Needs work
catch’s picture

catch
he/him
Primary language English
commented

Working on it, MR shortly.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

OK the problem is that when the tablesort CSS is moved to the library, Claro's classy tablesort.css, which specifies a td.is-active background color, is loaded after Claro's tables.css, which specifies background: none;

Adding an MR that just removes the declaration from classy.css - it makes no sense for Claro to define two different CSS rules for the same class, the classy one was just never getting used until the regression.

More generally, Claro has tablesort.css and tables.css, but defines various styles for tablesort inside tables.css. So it would be worth a follow-up to move all of that styling to tablesort.css to avoid unused CSS when regular, unsortable, tables are rendered.

catch opened merge request !11885

catch’s picture

catch
he/him
Primary language English
commented

Opened #3520057: Clean up Claro tablesort CSS.

nicxvan’s picture

nicxvan commented

Explanation is great, fix is simple, there is one test that looks like a random failure I cannot rerun.

If that passes I think this is rtbc.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Doubt the change didn’t break the build step

  • nod_ committed 7dc9bf7c on 11.x 
    Issue #3432244 by catch, lauriii, smustgrave, nicxvan: Follow-up Split...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 7dc9bf7 and pushed to 11.x. Thanks!

nod_ closed merge request !11885

Status: Fixed » Closed (fixed)

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