feat(table): added support for sticky column on right

[mattnolting]

closes #5391

This PR adds right: var(--pf-c-table__sticky-column--Right, revert); to support righthand sticky column. If the table cell is the first child, --pf-c-table__sticky-column--Left is set to 0. If the table cell is the last child, --pf-c-table__sticky-column--Right is set to 0. There are no other instances in which either value should be 0.

[patternfly-build]

Preview: https://patternfly-pr-5393.surge.sh

A11y report: https://patternfly-pr-5393-a11y.surge.sh

[mcoker]

Left a comment - I'm wondering if the move to targeting the first-last child might break someone.

[mcoker]

Just another option. I'm not tied to either way, but figured I'd mention it

---

I'm not super familiar with sticky tables, but is it a use case that a sticky column could not be the first child of the row? Either the previous children are hidden in some way or maybe aren't sticky, but some child in the middle of the row is? Here is an example with the third column sticky- https://codesandbox.io/s/great-christian-69uy1g?file=/index.htmlhttps://codesandbox.io/s/lucid-ardinghelli-7fnkro?file=/style.css

Another use case is where multiple columns are sticky, with regular positioned columns in-between. Here is an example with the first and third columns sticky - https://codesandbox.io/s/great-christian-69uy1g?file=/index.html

Those will break with this update, since it's going into main.

It looks like the first sticky (not necessarily first child) should be left: 0 by default, and any subsequent sticky (left) columns should have a left value set - assuming so, I wonder if it's better with a default of left: 0. I know that will mess up a right sticky column, since that will need to be unset, but with the page component, we have pf-m-sticky-top and pf-m-sticky-bottom. I'm wondering if maybe we could have left and right sticky, or maybe just .pf-m-sticky-column is left, and pf-m-sticky-column-right is right? Either now (to avoid breaking things), or for the long-term. I'm thinking it could be both, but I'm curious your opinion.

Looking at the react API, we set this for positioning:

First sticky column

isStickyColumn

Sticky siblings

isStickyColumn 
stickyLeftOffset="120px" // default is "0" from CSS

With this change as it is in the PR, I'm assuming we might do something like this?

First sticky column (on either side)

// Left
// ----
isStickyColumn 
// stickyLeftOffset="0" - would be needed if first sticky column isn't first-child

// Right
// -----
isStickyColumn 
// stickyRightOffset="0" - would be needed if first sticky column isn't last-child

Sticky siblings

// Left
// ----
isStickyColumn 
stickyLeftOffset="120px" // default is unset

// Right
// ----
isStickyColumn 
stickyRightOffset="120px" // default is unset

If we left the existing way it works as it is, and enabled isStickyColumnRight (or whatever), that could unset left, and set right: 0; and you'd have this API, and the codesandboxes I posted above would work as is.

First sticky column

// Left
// ----
isStickyColumn 

// Right
// -----
isStickyColumnRight

Sticky siblings

// Left
// ----
isStickyColumn 
stickyOffset="120px" // --pf-c-table__sticky-column--offset applies to left or right based on the modifier class

// Right
// ----
isStickyColumnRight
stickyOffset="120px" // --pf-c-table__sticky-column--offset applies to left or right based on the modifier class

To keep it from being breaking, we could keep stickyLeftOffset as it is and remove in v5, if that seems like an ok approach.

[mattnolting]

Since React is resetting left/right variables, I think we can move this to v5 pf-c-table__sticky-left/right

[mcoker]

In the CSS, I see __sticky-left, __sticky-right grouped with common styles, then both classes modify the same left property later in different selectors, and the vars are named --pf-c-table__sticky--cell - that says to me that sticky is the element, the sticky direction is the modifier, and then the vars could match:

[...]
--pf-c-table__sticky-header--Left: auto;
--pf-c-table__sticky-header--Right: auto;
--pf-c-table__sticky-header--m-left--Left: 0;
--pf-c-table__sticky-header--m-right--Right: 0;
[...]

.pf-c-table__sticky-header {
  // common styles
  position: var(--pf-c-table__sticky-header--Position);
  left: var(--pf-c-table__sticky-header--Left); // or just omit this
  right: var(--pf-c-table__sticky-header--Right); // or just omit this
  [...]
  
  &.pf-m-left {
    --pf-c-table__sticky-header--Left: var(--pf-c-table__sticky-header--m-left--Left); // or style as `left: var(--pf-c-table__sticky-header--m-left--Left);` if `left` is omitted on the parent.
  }

  &.pf-m-right {
    --pf-c-table__sticky-header--Right: var(--pf-c-table__sticky-header--m-right--Right); // or style as `right: var(--pf-c-table__sticky-header--m-right--Right);` if `right` is omitted on the parent.  }
}

That also implies a sticky element can be either variant, which I don't see why it couldn't be - in the same way sections and things are sticky when you reach them in scrollable regions - https://codesandbox.io/s/recursing-tristan-co5vc4?file=/fonts.css

Or are there other reasons you think they should be separate BEM elements?

[mcoker]

unrelated - does this need to be scoped in .pf-c-table?

[mattnolting]

Yes, it needs a higher specificity

[mcoker]

In the CSS, I see __sticky-left, __sticky-right grouped with common styles, then both classes modify the same left property later in different selectors, and the vars are named --pf-c-table__sticky--cell - that says to me that sticky is the element, the sticky direction is the modifier, and then the vars could match:

[...]
--pf-c-table__sticky-header--Left: auto;
--pf-c-table__sticky-header--Right: auto;
--pf-c-table__sticky-header--m-left--Left: 0;
--pf-c-table__sticky-header--m-right--Right: 0;
[...]

.pf-c-table__sticky-header {
  // common styles
  position: var(--pf-c-table__sticky-header--Position);
  left: var(--pf-c-table__sticky-header--Left); // or just omit this
  right: var(--pf-c-table__sticky-header--Right); // or just omit this
  [...]
  
  &.pf-m-left {
    --pf-c-table__sticky-header--Left: var(--pf-c-table__sticky-header--m-left--Left); // or style as `left: var(--pf-c-table__sticky-header--m-left--Left);` if `left` is omitted on the parent.
  }

  &.pf-m-right {
    --pf-c-table__sticky-header--Right: var(--pf-c-table__sticky-header--m-right--Right); // or style as `right: var(--pf-c-table__sticky-header--m-right--Right);` if `right` is omitted on the parent.  }
}

That also implies a sticky element can be either variant, which I don't see why it couldn't be - in the same way sections and things are sticky when you reach them in scrollable regions - https://codesandbox.io/s/recursing-tristan-co5vc4?file=/fonts.css

Or are there other reasons you think they should be separate BEM elements?

[mattnolting]

@mcoker Updated. sticky-header is being used to define a thead, so sticky-cell is more descriptive. LMK your thoughts.

[mcoker]

Just one nit - LGTM!

[patternfly-build]

🎉 This PR is included in version 5.0.0-alpha.29 🎉

The release is available on:

• GitHub release
• npm package (@alpha dist-tag)

Your semantic-release bot 📦🚀