Make drupal.tableheader only use CSS for sticky table headers

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Don't think this is good to go. You can't remove and deprecate at the same time.

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.

The MR should target 11.x, it will be backported to 10.3.x from there. Couple of extra comments on the MR. This looks great in general, so much code to get rid of.

Small metadata change - putting this into JavaScript because it's mostly about getting rid of JavaScript, might be questionably but close to that than theme system. Also re-titling for the new proposed solution.

Will need an issue summary update to match the new solution as well as a change record.

Making the title easier to read

One comment on the MR - because stable9 is there for backwards compatibility, we should probably add the new class and retain the old class instead or replacing. Otherwise this looks great to me!

Just rerolled patch #15 for D10.2.5.
Now that I have realized, that there are problems with this patch, I see the comment "Do not use this patch" at #15. So please ignore this too

Tested with Drupal 11.x
The header is sticky on scroll(see the attached video).

+1 for RTBC

Reroll of patch in https://www.drupal.org/project/drupal/issues/3439580#comment-15549200 for D 10.2.6

This looks good to go, I updated the summary to be clearer.

Tests are passing and the video confirms it is working as expected as well.

The feedback on the MR has been addressed as well so I think it is ready.

Forgot to mention I also reviewed the change record which looks good too!

why the class name change? looks like it wouldn't cause a problem to use the sticky-enabled class to apply the css instead of creating a new one.

The original js adds the sticky-header class, and this is what the existing CSS targets, so it's not actually creating a new class.

this.$stickyTable = $(
          '<table class="sticky-header" style="visibility: hidden; position: fixed; top: 0;"></table>',

core/modules/system/css/components/sticky-header.module.css

table.sticky-header {
  z-index: 500;
  top: 0;
  margin-top: 0;
  background-color: #fff;
}

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.

hiding patch to prevent the bot from messing with the issue

@#35 thanks for the details, all good then!

It will either need to be 11.1 for removal in 12 or 10.4 for removal in 12 depending on whether we backport this to 10.4 or not. I am not entirely sure if we would or not - it might be useful for themes that want to override the component to backport this to keep things in sync, but it's otherwise going to be quite a transparent change for most sites I would think which could be an argument for or against a backport. Will get an opinion or two from other committers.

let's get this in, risk is low

Committed and pushed to 11.x

fixing issue credits

Hi,

Glad to see JS features made available with CSS only!

Sorry to arrive after the party, but in https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/modules/vie...

    $form['sticky'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Enable Drupal style "sticky" table headers (JavaScript)'),
      '#default_value' => !empty($this->options['sticky']),
      '#description' => $this->t('(Sticky header effects will not be active for preview below, only on live output.)'),
    ];

Should the (JavaScript) be removed too?

@GrimReaper - good find, could you open follow-up issue for fixing that text?

Thanks @catch.

I am on it :)

Follow up issue created: #3451738: Remove JavaScript from Views configuration form now that it is only CSS

MR created and pipeline green, ready for review.

Hi,

After this change, a lot of people seems to run into this issue

InvalidArgumentException: The callable definition provided "[Drupal\claro\ClaroPreRender,tablePositionSticky]" is not a valid callable. in Drupal\Core\Utility\CallableResolver->getCallableFromDefinition() (line 69 of core/lib/Drupal/Core/Utility/CallableResolver.php).

Is there any solution for it ?

Thank you