Olivero: Z-index issue with table with sticky header

Re-rolled patch #10 for 10.1.x. please review

Tested patch #15 with Drupal 10.1.x and it broke the site layout.
Attaching gif for after applying patch #15.

Please refer to attached gif for the same.
Changing status to needs work.

Applied patch #15 but after applying layout was borken.
For ref attached two recording.

Applied Patch cleanly. But table header sticky is not working.

@Vinayak.Ambig Thanks for the review, and welcome to the community! While the video would normally be helpful as a patch review, there are already two comments reviewing patch #15 asserting that the site layout is broken. Please take a look at the Drupal Issue Etiquette → .

Since this patch has already been reviewed, we want this issue to remain as "needs work".

inset-block-start causes the issue, I have posted a patch which fixes the issue but I will try to come up with a better solution for this issue

A few issues with #20:

custom commands failure - be sure to run yarn lint:core-js-passing to make sure the JS file will pass the linting tests.

Let's avoid overwriting the entire style attribute by using the element's style property:

      if (pinnedState === true) {
        siteHeaderFixable.setAttribute('data-offset-top', '');
        siteHeaderFixable.setAttribute('style','inset-block-start: auto;');
      }
      else {
        siteHeaderFixable.removeAttribute('data-offset-top');
      }

siteHeaderFixable.style.insetBlockStart = 'auto';

Instead of attaching the sticky tableheader JS to the Olivero global library, we should do a library-override. See the olivero.info.yml and olivero.libraries.yml files and look to the drupal.message library to see an example of this.

Tried to fix custom commands failure of #20 patch

Tested #22 and it doesn't fix the suggestions made in #21, changing the status to needs work again.

@Andy-blum thanks for the suggestions, I've changed the style attribute usage as per your suggestion. Im not sure why we need library-override because we are not extending / overriding any external library and the js/navigation-utils.js: {} that does the toogle function is a part of the global styling. Let me know your thoughts

@ameymudras

Im not sure why we need library-override because we are not extending / overriding any external library and the js/navigation-utils.js: {}

We are not currently doing this, but we have a hidden dependency there. For example in the code below:

  function adjustStickyTableHeader() {
    Drupal.TableHeader.tables.forEach(function (table) {
      table.recalculateSticky();
    });
  }

The Drupal.TableHeader object is coming from core/misc/tableheader.js, which is only present when the core/drupal.tableheader library is attached. Olivero does not declare any dependencies on this library, so there's the possibility that Olivero attaches javascript that will break when core JS has not been also been attached.

We do a similar thing with the core/drupal.message library, which is why I pointed to it as an example.

In olivero.info.yml we have

libraries-extend:
  core/drupal.message:
    - olivero/drupal.message

This tells Drupal that when Olivero is the theme, and the core/drupal.message library is attached, we need to also attach the olivero/drupal.message library.

Then, in olivero.libraries.yml we have

drupal.message:
  version: VERSION
  js:
    js/message.theme.js: {}
  dependencies:
    - olivero/messages

Saying that when we attach olivero.drupal.message, we need to send some additional JS as well as pull in the olivero/messages library.

For this issue, we need to add a library-extend entry for core/drupal.tableheader, and then a new library that serves the files Olivero needs to make this fix work.

Moving to NW for #25 points.

Also was tagged for tests which still appear to be needed.