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

Fixed scrollbars appearing when they're not needed #767

Merged
merged 1 commit into from
Feb 21, 2021
Merged

Fixed scrollbars appearing when they're not needed #767

merged 1 commit into from
Feb 21, 2021

Conversation

Narcha
Copy link
Contributor

@Narcha Narcha commented Feb 2, 2021

When a table has fixedHeader enabled and not enough entries to fill it's max-height, a disabled scrollbar appears.
I fixed this by replacing overflow-y: scroll with overflow-y: auto so the scrollbar only appears when it's necessary.

I've tested a few other configurations to see if this change breaks anything,
I'm not aware of any negative impact this could have.
Please let me know if I missed anything.

@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #767 (ad5d0c7) into master (c33175e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #767   +/-   ##
=======================================
  Coverage   99.38%   99.38%           
=======================================
  Files          43       43           
  Lines         490      490           
  Branches      138      138           
=======================================
  Hits          487      487           
  Misses          3        3           
Impacted Files Coverage Δ
src/DataTable/TableBody.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c33175e...ad5d0c7. Read the comment docs.

@jbetancur
Copy link
Owner

Thank you for PRing this. Sorry for the delay, but I still need some time to test it out. Also, we will need to PR this into the next branch as well.

@jbetancur jbetancur self-assigned this Feb 21, 2021
@jbetancur jbetancur added feature next The next version (beta) labels Feb 21, 2021
@jbetancur jbetancur merged commit 9786ac0 into jbetancur:master Feb 21, 2021
@jbetancur
Copy link
Owner

This looks good! I'll also cherry pick this change into the next branch as well

@Narcha
Copy link
Contributor Author

Narcha commented Feb 21, 2021

One thing to note:
If #586 gets fixed by somehow adding the correct amount of padding-right to rdt_TableHeadRow to compensate for the scrollbar width, this will have to be reevaluated.

@jbetancur
Copy link
Owner

@Narcha I've moved #586 to next as well. I've been meaning to fix that issue for some time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature next The next version (beta)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants