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

Table height does not account for horizontal scrollbar #2525

Merged
merged 8 commits into from
Jan 30, 2025
Merged

Conversation

mollykreis
Copy link
Contributor

@mollykreis mollykreis commented Jan 29, 2025

Pull Request

🀨 Rationale

Fixes #2520

πŸ‘©β€πŸ’» Implementation

The issue is that when the horizontal scrollbar was present while styling the table with a height of tableFitRowsHeight, a vertical scrollbar would be present because the height token's value didn't account for the height of the horizontal scrollbar. Now, the virtualizer's ResizeObserver hander calculates the height of the horizontal scrollbar (which is 0 if there isn't a horizontal scrollbar), and exposes that value as an @observable, similar to scrollHeight. As a result, the table can use both values in its calculation of tableFitRowsHeight, resulting in no vertical scrollbar when using the token (unless the max-height of the table is reached).

πŸ§ͺ Testing

Manually tested, including the following scenarios to ensure the auto height of the table adjusted correctly as a horizontal scrollbar was added/removed:

  • toggle the value of column-visible on a column
  • add/remove a column from the DOM
  • increase/decrease the minimum width of a column
  • resize browser window

New auto tests, including chromatic test that loads a table with auto height and a horizontal scrollbar

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@rajsite rajsite changed the title Table height Table height does not account for horizontal scrollbar Jan 29, 2025
@mollykreis mollykreis marked this pull request as ready for review January 30, 2025 15:22
@mollykreis mollykreis requested a review from jattasNI as a code owner January 30, 2025 15:22
Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified in macOS Safari and Firefox (both of which show overlay scrollbars) that the table fit rows height stories behave correctly. It doesn't reserve extra space for the horizontal scrollbar and it doesn't cause an unnecessary vertical scrollbar to appear when the horizontal one is shown. πŸš€

@mollykreis mollykreis enabled auto-merge (squash) January 30, 2025 19:11
@mollykreis mollykreis merged commit 1ae9e67 into main Jan 30, 2025
12 checks passed
@mollykreis mollykreis deleted the table-height branch January 30, 2025 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table 'ni-nimble-table-fit-rows-height' token doesn't account for horizontal scrollbar
4 participants