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

Hide empty cells using CSS #391

Merged

Conversation

martinRenou
Copy link
Member

Should fix #387

@jtpio jtpio changed the title Filter empty cells [WIP] Filter empty cells Oct 2, 2019
@martinRenou martinRenou force-pushed the ignore_empty_cells branch 2 times, most recently from a75db57 to 3b382c7 Compare October 2, 2019 14:48
@martinRenou martinRenou changed the title [WIP] Filter empty cells Filter empty cells Oct 2, 2019
@martinRenou martinRenou changed the title Filter empty cells Hide empty cells using CSS Oct 2, 2019
Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Totally agree with this. I think we stripping cells without output in the code was more like a temporary solution, this is what we had to do from the beginning.

voila/handler.py Outdated Show resolved Hide resolved
@vidartf
Copy link
Contributor

vidartf commented Oct 7, 2019

Are there not some cases where a cell can initially have no outputs, but some later code (asynchronously) adds an output to the cell? This would not normally be an issue with nbconvert, but if we are doing progressive rendering, this might be an issue. Is it possible to use the :empty CSS selector here?

@martinRenou
Copy link
Member Author

Good point, I agree using :empty or :blank would be better, because it would handle this case.

@vidartf
Copy link
Contributor

vidartf commented Oct 7, 2019

:blank is still in the future: https://caniuse.com/#feat=mdn-css_selectors_blank

@martinRenou
Copy link
Member Author

I've got an alternate solution, I'm going to open another PR

@martinRenou
Copy link
Member Author

Alternate solution: #405

@martinRenou martinRenou changed the title Hide empty cells using CSS WIP - Hide empty cells using CSS Oct 8, 2019
@martinRenou
Copy link
Member Author

@vidartf

Are there not some cases where a cell can initially have no outputs, but some later code (asynchronously) adds an output to the cell?

Do you already have a specific use case in mind? I'm trying to figure out a scenario where this could happen.

@maartenbreddels
Copy link
Member

display supports a display_id, so that you can change outputs of previous cells. e.g.:
https://nbviewer.jupyter.org/github/jupyter/nbconvert/blob/master/nbconvert/preprocessors/tests/files/Clear%20Output.ipynb

We don't support that actually with voila progressive rendering. I wonder if we should explicitly not support that, or if we should fix this somehow.

Note that if you output an empty Output widget, it will always work (since the cell contains output in the form of a widget)

@martinRenou
Copy link
Member Author

display supports a display_id, so that you can change outputs of previous cells

I also thought about this, but for having a display_id you need to display something first no? With a first display call

@vidartf
Copy link
Contributor

vidartf commented Oct 8, 2019

I also thought about this, but for having a display_id you need to display something first no? With a first display call

Yes, my question basically boils down to: If you do a a display call without any data, does that still make a difference for cell.outputs?

@martinRenou
Copy link
Member Author

Yes, my question basically boils down to: If you do a a display call without any data, does that still make a difference for cell.outputs?

I actually tried and it seems you can not use display with no data. I thought of making an output placeholder with display(None) and it actually displays the string "None", so there is an output.

@vidartf
Copy link
Contributor

vidartf commented Oct 8, 2019

This seems to work: display()

The interesting thing is that if I try this:

> handle = display(display_id=True)

> handle.update('world')

It doesn't actually work in neither classic notebook nor lab. No errors are raised, but the update isn't actually displayed. So I don't think it is a very common pattern (but strictly speaking, I would have expected that it should work).

@martinRenou
Copy link
Member Author

Interesting. I agree it should work.

@vidartf
Copy link
Contributor

vidartf commented Oct 8, 2019

(note, that I'm happy with the code in this PR as-is, since neither classic nor lab supports it, just making a note for the future about it)

@martinRenou
Copy link
Member Author

I do agree.

I think I prefer the alternate solution though #405. It's basically the same behavior, only that instead of hiding empty cells we simply don't send them.

@vidartf vidartf mentioned this pull request Oct 11, 2019
@martinRenou martinRenou force-pushed the ignore_empty_cells branch 2 times, most recently from 97d8919 to 28e0252 Compare October 16, 2019 15:47
@martinRenou martinRenou changed the title WIP - Hide empty cells using CSS Hide empty cells using CSS Oct 16, 2019
@martinRenou
Copy link
Member Author

@maartenbreddels would you like to have another look at this one?

@SylvainCorlay
Copy link
Member

I am fine with this solution.

@SylvainCorlay
Copy link
Member

@maartenbreddels I had a small comment on this one that martin is fixing now. I think this should get in before the release.

@maartenbreddels
Copy link
Member

I think so too, failure is unrelated. I'll wait a bit more before making a release, in case we need to merge more.

@martinRenou
Copy link
Member Author

Done

@SylvainCorlay SylvainCorlay merged commit 66d1a7c into voila-dashboards:master Oct 17, 2019
@martinRenou martinRenou deleted the ignore_empty_cells branch October 17, 2019 13:43
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.

Extra padding in empty cells
4 participants