Take scrollbars into account when computing Dataframe dimensions #2622
Conversation
vdonato
force-pushed
the
df_scrollbar
branch
2 times, most recently
from
49778b8 to
1a4025a
Compare
|
@asaini: My PR comment gives a pretty good description of the changes made, but the TL; DR version of the changes to look for vs the current behavior on the
|
akrolsmir
left a comment
akrolsmir
left a comment
There was a problem hiding this comment.
Looks great! To get the snapshot tests to pass, you'll want to grab the failing diff.png and run it through the diffmazing cropper, then put the new snapshot in this PR.
kmcgrady
left a comment
kmcgrady
left a comment
There was a problem hiding this comment.
The solution is less than ideal, but I think might have to be the path.
In terms of unit testing, here are some ideas that might work:
- you can use enzyme's
mount(or fromlib/test_util) and utilize the getDOMNode and verify the offset and clientWidth are the same. - If that doesn't work, the e2e test should be able to resolve this as well, both in a snapshot test (the test should not display scrollbars) and doing the same check for
offsetWidthandclientWidth
vdonato
force-pushed
the
df_scrollbar
branch
3 times, most recently
from
972729d to
f957dfe
Compare
vdonato
left a comment
vdonato
left a comment
There was a problem hiding this comment.
Thanks for the reviews @akrolsmir, @kmcgrady! May be worth taking a second glance at things to make sure the tests seem sane.
|
Looks reasonable, just got to update the snapshot for the cypress test. Let me know if you need help with that. And then @asaini should take a look at it for product review. |
vdonato
force-pushed
the
df_scrollbar
branch
4 times, most recently
from
ed58f39 to
ad0d687
Compare
At least this is a pretty strong incentive to fix the currently-broken Docker image for running cypress 😐
|
LGTM 👍 |
* develop: Take scrollbars into account when computing Dataframe dimensions (streamlit#2622)
* develop: ReportContext constructs its own widget_ids_this_run (streamlit#2669) Add types to report_thread.py (streamlit#2665) Take scrollbars into account when computing Dataframe dimensions (streamlit#2622) Revert "Revert "Revert "Add anchors to Markdown headers (streamlit#2513)""" (streamlit#2664) ♻️ Remove "_proto" from "data_frame_proto.py" (streamlit#2640)
4 participants
It turns out the bug in #2543 appears only when there are enough elements in a Dataframe for a scrollbar to be necessary to display all the data. In the case where we need a vertical scrollbar, the width that we provide to the
MultiGridwe're using to render our Dataframe ends up being slightly too small due to the presence of the scrollbar. This causes a horizontal scrollbar to appear. The other case is analogous.To fix this, we borrowed a helper function from react-bootstrap/dom-helpers for getting the width of scrollbars, then used the helper to fix our rendering dimensions calculations to take scrollbars into account.
Closes #2543