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

reenable minimum height for voila #2588

Closed
wants to merge 1 commit into from
Closed

Conversation

timkpaine
Copy link
Member

@timkpaine timkpaine commented Apr 5, 2024

Tweaks and partially reverts #2352

Currently, Perspective works great in voila except that it does not have any height::

Screenshot 2024-04-05 at 17 57 46

This PR gives it a minimum height:
Screenshot 2024-04-05 at 17 57 26

The sentiments in the original PR still hold, I'm looking for an alternative way to tell if specifically in voilà. but with voila-dashboards/voila#1457 we can now disambiguate and apply only in voilà

@timkpaine
Copy link
Member Author

timkpaine commented Apr 5, 2024

@timkpaine timkpaine marked this pull request as ready for review April 9, 2024 18:23
@timkpaine
Copy link
Member Author

Not done in this PR, but we could use the same idea for notebook, which might help if/when we work in e.g. vs code: https://github.com/jupyter/notebook/blob/3b8e6736075279f80cd1e590c71a872dc5ad2c11/notebook/templates/notebooks.html#L15

@timkpaine
Copy link
Member Author

needs #2596

Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good!

I will merge this as-is, but I have to ask, what is the purpose of all of these custom height overrides? Is it because we want 100% to be the default in JupyterLab's "Output View" mode? Can we just change it to be e.g.:

div.PSPContainer {
    height: 520px;
}

:not(.jp-NotebookPanel-notebook) div.PSPContainer {
    height: 100%;
}

I need to ask, because this feature has no tests, and I don't know how to build a voila app. Also, the PR has the word "reenable" in the title, as in, this feature has regressed already ...

@texodus texodus added the bug Concrete, reproducible bugs label May 15, 2024
@timkpaine
Copy link
Member Author

timkpaine commented May 15, 2024

In voila (and vs code and jupyterlab and Jupyter notebook for that matter) the output container expands to the size of its contents, so if you just set height: 100% you'll end up with a height 0 div since perspective tries to expand to fill its container and does not in-and-of-itself have a minimum height. Thus we need to give the container div some minimum height so that it displays, and 520px seemed to be a good heuristic from a UX standpoint (e.g. its pretty close to the default pandas max height when it collapses intermediate rows).

You can run a voila app with voila <notebook.ipynb> and it will run locally, I don't think it really requires tests since for all these various platforms we just need to find the container element and set a height next to the other container blocks.

Your change is fine but I think there is a reason for how we do it (though I am not aware of the reason 😄). I know we set height 98% in the viewer so we have room for that resize part (in case someone wants to make the perspective widget larger than 520px). Im ok with setting an absolute div.PSPContainer { height: 520px } but im unsure of the implications across outputs we don't know about (e.g. other than Jupyter Notebook, JupyterLab, Voilà, and VS Code, which are the 4 we've been tracking).

@texodus texodus mentioned this pull request May 19, 2024
@texodus
Copy link
Member

texodus commented May 19, 2024

I've cherry-picked this into #2623, closing in favor of this PR.

@texodus texodus closed this May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants