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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix table overflow styling #4934

Merged
merged 7 commits into from
Jul 7, 2022

Conversation

mayagbarnes
Copy link
Collaborator

@mayagbarnes mayagbarnes commented Jul 6, 2022

馃摎 Context

Currently, st.table content overlaps beyond the parent container - this PR fixes the issue by styling the overflow content to be scrolled for both arrow/legacy.

  • What kind of change does this PR introduce?

    • Bugfix

馃 Description of Changes

  • Add overflow: "auto" to the styled components for ArrowTable and Table
    • This is a visible (user-facing) change

Revised:
Screen Shot 2022-07-06 at 2 22 16 PM

Current:
Screen Shot 2022-07-06 at 2 22 45 PM

馃И Testing Done

  • Screenshots included
  • Added/Updated e2e tests

馃寪 References

Does this depend on other work, documents, or tickets?


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@mayagbarnes mayagbarnes marked this pull request as ready for review July 7, 2022 07:58
@mayagbarnes mayagbarnes requested a review from a team July 7, 2022 07:58
@LukasMasuch
Copy link
Collaborator

Do you know why the height in the table snapshots is influenced by this change? It is probably requiring some additional space for the scrollbars, or?

@mayagbarnes
Copy link
Collaborator Author

mayagbarnes commented Jul 7, 2022

That's correct! The scrollbar itself doesn't show since there is no cursor hovering over the table, but the "auto" styling applies a scrollbar to the test based on the overflow content.

Code from test script:
Screen Shot 2022-07-07 at 9 59 09 AM

Copy link
Collaborator

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

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

LGTM 馃憤 But I think we might be able to prevent the additional spacing:

I saw a recent task from @jrieke to have all top/bottom paddings standardized (including tables, charts, and data frames). We are probably fine to add this right now since it is not standardized yet, but maybe overlay is a better way to do this since it won't add any extra space for the scrollbar (at least in webkit browsers). We are using this in a few places in the codebase, e.g.:

My suggestion would be to change it to:

overflowX: ["auto", "overlay"]

overflowX since we are mainly trying to fix the vertical dimension, or? If overlay does not work, I'm also fine with auto.

@mayagbarnes mayagbarnes merged commit 4c39606 into streamlit:develop Jul 7, 2022
@mayagbarnes mayagbarnes deleted the fix-table-scroll branch July 7, 2022 22:21
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.

Set overflow to scroll for expander content DataFrames in columns intermittently overlapping.
3 participants