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

overflow and scrollbars #8

Closed
3 tasks done
lfoppiano opened this issue Nov 30, 2023 · 7 comments · May be fixed by #11
Closed
3 tasks done

overflow and scrollbars #8

lfoppiano opened this issue Nov 30, 2023 · 7 comments · May be fixed by #11
Assignees
Milestone

Comments

@lfoppiano
Copy link
Owner

lfoppiano commented Nov 30, 2023

When the component is loaded, it pushes the scrolling back down. We should add an option that allow the component to have a overflow:scrolling.

Tasks

  • The user does not set any parameters (e.g. pdf_viewer(file)) -> we unwrap the PDF (no scroll bars, every page one after the other)
  • The user does set height = 700 (absolute number) -> then we show the scrollbars and limit the window to the height
  • The user does set height = 80% (relative number) -> same as previous, show the scrollbars and limit the window to the height because streamlet.setFrameHeight doesn't support relative number
@t29mato
Copy link
Collaborator

t29mato commented Nov 30, 2023

set-scrollbar-and-overflow.mov

@lfoppiano
Copy link
Owner Author

lfoppiano commented Nov 30, 2023

looks good.

We want to have both the previous and this behaviour, selected by parameters, I think when the user (User = anybody/developers that uses the component) passes a height value:

  1. The user does not set any parameters (e.g. pdf_viewer(file)) -> we unwrap the PDF (no scroll bars, every page one after the other)
  2. The user does set height = 700 (absolute number) -> then we show the scrollbars and limit the window to the height
  3. The user does set height = 80% (relative number) -> same as previous, show the scrollbars and limit the window to the height

@t29mato
Copy link
Collaborator

t29mato commented Jan 12, 2024

The user does set height = 80% (relative number) -> same as previous, show the scrollbars and limit the window to the height

The argument of Streamlit.setFrameHeight() supports only numbers, not str like 80%, so to support this feature, we need to calculate from the user's window size, which is not sure it can make it.

The user does not set any parameters (e.g. pdf_viewer(file)) -> we unwrap the PDF (no scroll bars, every page one after the other)

I can make it. it would take 1 - 4 hours.

@lfoppiano
Copy link
Owner Author

The argument of Streamlit.setFrameHeight() supports only numbers, not str like 80%, so to support this feature, we need to calculate from the user's window size, which is not sure it can make it.

Let's drop it then, I can validate the parameter in the python part.

I can make it. it would take 1 - 4 hours.

Isn't it already the current behaviour?

@t29mato
Copy link
Collaborator

t29mato commented Jan 17, 2024

I can make it. it would take 1 - 4 hours.

Isn't it already the current behaviour?

Yeah, that's true. now, it's handling both cases

9196b85

@lfoppiano
Copy link
Owner Author

@t29mato could you please summarise what exactly should be tested? The point number 2 is it implemented?
Is this implemented in the main or as a PR?

@t29mato
Copy link
Collaborator

t29mato commented Jan 18, 2024

summarise what exactly should be tested?

  1. Test for Unset height (Default Behavior):

    • When height is not set (i.e., None), the PDF should display in its entirety without any overflow.
  2. Test for Set height (Numeric Value):

    • If a numeric value is assigned to height, the PDF should display with the specified height in pixels, and overflow should be enabled.
  3. Test for Invalid height Input:

    • The function should throw an error if height is given a value other than a number or None.

The point number 2 is it implemented?

Is point number 2 The user does set height = 700 (absolute number) ? Then, yes, I've implemented it.

Is this implemented in the main or as a PR?

Sorry, I don't understand this.

@lfoppiano lfoppiano added this to the 0.0.4 milestone Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants