Skip to content

Fix bottom scrollbar behavior#307

Closed
bleakley wants to merge 3 commits intomasterfrom
bottom-scrollbar-fix
Closed

Fix bottom scrollbar behavior#307
bleakley wants to merge 3 commits intomasterfrom
bottom-scrollbar-fix

Conversation

@bleakley
Copy link
Copy Markdown
Contributor

I didn't add a lot of description to #300 but what I was attempting to fix was the behavior where the scrollbar would erratically appear as the table was resizing above the minimum column widths, and would sometimes stay visible in certain positions where it shouldn't when columns are still above their minimum widths and there is actually no horizontal overflow to scroll through.

That PR fixed the scrollbar issue but created a visible gap on the left of the table. I wanted a css-only solution but I have a simple js solution that should fix all the problems. We just determine what the minimum width of the full table will be based on the minimum width of each column, and if the table-scroll div width is ever greater than that we hide the scrollbar.

@bleakley bleakley requested a review from severo October 21, 2025 18:31
Copy link
Copy Markdown
Contributor

@severo severo left a comment

Choose a reason for hiding this comment

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

hmmm, it becomes a bit complex, but why not if it's the only way.

In any case, can we move the code to useColumnWidths?

@bleakley bleakley requested a review from severo October 22, 2025 17:24
Copy link
Copy Markdown
Contributor

@severo severo left a comment

Choose a reason for hiding this comment

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

sorry it's complex, and there are no comments, I have a hard time understanding the code; And do you have a video to show the issue and how it is solved by this PR?

})
}, [allColumnsParameters, isHiddenColumn])

const visibleColumnIndexes = useMemo(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit, but for consistency,

Suggested change
const visibleColumnIndexes = useMemo(() => {
const columnIndexes = useMemo(() => {

Comment on lines +164 to +167
useEffect(() => {
if (availableScrollerWidth === undefined) return
setScrollNeedInputs?.({ columnIndexes: visibleColumnIndexes, availableWidth: availableScrollerWidth, waitMs: 50 })
}, [availableScrollerWidth, visibleColumnIndexes, setScrollNeedInputs])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Iiuc, no need for a useEffect: see below

setAvailableWidth?.(tableWidth - leftColumnWidth)
const available = tableWidth - leftColumnWidth
setAvailableWidth?.(available)
setAvailableScrollerWidth(available)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand that we don't need the useEffect above, but:

Suggested change
setAvailableScrollerWidth(available)
setScrollNeedInputs?.({ columnIndexes: visibleColumnIndexes, availableWidth: available, waitMs: 50 })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and remove the availableScrollerWidth state

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and if so, I guess that we could have only one function call that replaces the lines

@bleakley
Copy link
Copy Markdown
Contributor Author

This is getting too complicated, and is trying to a fix a problem we probably don't need to worry about, I have a simpler and better explained PR here: #308

@bleakley bleakley closed this Oct 22, 2025
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.

2 participants