Refactor Claro's tabledrag stylesheet

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

I have added before/after screenshots for reference. please review

Before patch

After patch

Think some more nesting could be done. Especially with the tree classes.

Reran the tests and it seemed to be a random failure.

Nesting looks good

@nest needs to be removed

I did some additional significant refactoring. This stylesheet is a pain because tabledrag is a pain.

When we test this, we need to be extremely thorough. When testing in Safari (16.1 but also verified in 16.4), I noticed a pre-existing issue where the table rows are overly large. This patch fixes this too.

In regards to the @nest comments above with @nod_, we're having a discussion at https://drupal.slack.com/archives/C1BMUQ9U6/p1681135929045969 on whether we should be using it.

Still setting to NR since it definitely needs additional testing.

Applied Patch. It's working fine.
For ref attached screenshot.

Seems rebase failed to apply.

Just merged 10.1.x into the branch, and didn't have any conflicts.

Now it looks good.

There's still a regression on Safari where the table rows have a lot of extra padding after row has been moved:

Picking this up as part of the Claro Contribution day

Pushed a fix up for this to the existing MR, ready for review now.

Thanks

Seems to have an order issue

themes/claro/css/components/tabledrag.css
266:3 ✖ Expected "float" to come before "flex-direction" order/properties-order
themes/claro/css/components/tabledrag.pcss.css
244:5 ✖ Expected "float" to come before "flex-direction" order/properties-order
2 problems (2 errors, 0 warnings)

Ah, my mistake. For some reason those errors weren't showing when running `yarn lint:css`. Fixed now and passing checks.

Thanks for fixing that!

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

When we test this, we need to be extremely thorough.

That is from #12. I see some test results in #19 but no other indication that this change has been thoroughly tested. Therefore, I am tagging for manual testing. I had to restore the standard issue template so I could add testing as a remaining task. Setting to Needs review for testing.

There are two MRs here. Can someone add to the issue summary which one is to be reviewed and hopefully close the other one. Thanks.

Both the MR has same code the difference is one for against branch 10.1.x ( MR3517 ) and the other one for branch 11.x ( MR5829 )

smustgrave → changed the visibility of the branch 3332683-refactor-claros-tabledrag to hidden.

So I tested using the Table Drag test module in core.
Altered the route tabledrag_test.test_form to use the claro theme.

Everything appears to be lined up correctly.
I'm able to drag rows around without issue.
Highlighting appears to be the same with/without the MR.

There is change in the th alignement with this patch see screenshot. There is a width:auto missing apparently that is causing it.

Addressed #32, Padding-inline-start: 0; was missing on draggable td,th.

Believe I'm seeing the issue reported in #32 has been addressed.

hierarchical indicator looks broken

Replicated this issue in Firefox.
Fixed this and pushed it to the MR branch.

Tested in Firefox, Safari, Chrome (on Mac, don't have access to another machine right now)

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.

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

Added a rebase and resolved the merge conflicts. Please review.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

• Switched to 11.0.x branch
• Fixed stylelint

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.

VladimirAus → changed the visibility of the branch 11.x to hidden.

Rebase seems good.

nod_ → closed merge request !5829

Committed f1769fa and pushed to 11.x. Thanks!

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