Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IndexTable] Sticky header in first or last fixed column missing border and getting incorrect min-width #11042

Closed
chloerice opened this issue Oct 26, 2023 · 1 comment · Fixed by #11617
Assignees
Labels
Priority: Medium Non-blocking regression, noticable inconcistency, or important feature

Comments

@chloerice
Copy link
Member

Issue summary

The header of IndexTable is missing fixed column header border when scrolling, they're the wrong width, and the last column's header isn't sticky.

Expected behavior

First sticky header cell should:

  • Have correct width (needs to have padding-right 400 and min-width set on style prop should round up to the 1px)
  • Have border-right when scrolling

Last sticky header cell (when last column sticky) should:

  • Be sticky
  • Have correct width (needs to have padding-left 400 and min-width set on style prop should round up to the nearest 0.5px)
  • Have border-left when scrolling
Screenshot 2023-10-26 at 12 13 07 PM

Actual behavior

First sticky header cell:

  • Has incorrect width (has padding-right 300 and min-width set on style prop is 1px lower than column's width)
  • Doesn't have border-right when scrolling

Last sticky header cell (when last column sticky):

  • Isn't sticky (scrolls with the other header cells)
  • Has incorrect width (has padding-right 300 and min-width set on style prop is 0.5px lower than the column's width)
  • Doesn't have border-left when scrolling
Screenshot 2023-10-26 at 12 13 27 PM

Steps to reproduce the problem

Go to a store with an order list long enough to be scrolled so the IndexTable header is sticky, note the first column header cell is missing a left border and is not the correct width on inspection compared to the column data cells.

@chloerice chloerice added untriaged Priority: Medium Non-blocking regression, noticable inconcistency, or important feature Bug Something is broken and not working as intended in the system. Uplift polish Fast follow fixes and polish post Summer Editions and removed untriaged labels Oct 26, 2023
@lgriffee lgriffee added #gsd:38846 Admin Quality Improvements (Q1 2024) and removed Uplift polish Fast follow fixes and polish post Summer Editions labels Jan 12, 2024
@lgriffee lgriffee removed #gsd:37811 Bug Something is broken and not working as intended in the system. labels Jan 22, 2024
@jesstelford
Copy link
Contributor

jesstelford commented Feb 14, 2024

Looks like the sticky header has significantly diverged from the non-sticky header, and so it's missing logic and styles that the non-sticky header has since gained.

In addition to the OP screenshots, we noticed the sticky header's first column is always staying sticky when the non-sticky header doesn't due to insufficient horizontal space:

304627107-beb6a135-f86a-46b5-a7d1-32f7c806a232

After investigating with @gwyneplaine, it appears the best path forward is to re-use the logic & styles from non-sticky header in the sticky header. This should bring back consistency to fix the discovered issues and ensure it doesn't drift again.

Brain dumping some tasks:

### Tasks
- [x] ~Remove unused `.Table-sortable` background styles (Summer Editions made that redundant)~ Moved to: https://github.com/Shopify/polaris-internal/issues/1465
- [x] Add a `Tag` option to `renderHeading()` to toggle between `div` / `hr`
- [x] Remove `.StickyTableColumnHeader` styles
- [x] ~Consolidate `.StickyTableHeader` styles with `.Table` styles~ _(out of scope)_
- [x] Consolidate DOM element width calculations / `min-width` setting inside `resizeTableHeadings()`
- [x] Search for selectors targeting removed classes
- [x] Test with Bulk actions
- [x] Test with loading state
- [x] Test when there is/isn't enough width to render the sticky columns (Add story variants for this? Maybe by rendering in a fixed-size scrollable container?)
- [x] Test when `unselectable === true`
- [x] Test / compare accessibility
- [x] Test sortability

@lgriffee lgriffee removed the #gsd:38846 Admin Quality Improvements (Q1 2024) label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium Non-blocking regression, noticable inconcistency, or important feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants