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

Delete html fallback message. #2007

Merged
merged 3 commits into from Mar 23, 2018

Conversation

Projects
None yet
3 participants
@jasongrout
Member

jasongrout commented Mar 20, 2018

This reverts #1674, since we discovered some problems with the classic notebook and html outputs.

Fixes #1777
Fixes #1951

Delete html fallback message.
This reverts #1674, since we discovered some problems with the classic notebook and html outputs.

Fixes #1777
Fixes #1951
@jasongrout

This comment has been minimized.

Member

jasongrout commented Mar 20, 2018

I'll also add a docs page explaining what to do if you see this, but were hoping to see widgets, basically containing the helpful text here.

@jasongrout jasongrout added this to the 7.2 milestone Mar 20, 2018

@jasongrout jasongrout requested review from vidartf, pbugnion and mwcraig Mar 20, 2018

@vidartf

This comment has been minimized.

Member

vidartf commented Mar 21, 2018

Note: The repr in the plain text mimetype isn't always nice and clean (it can become quite large for complex widgets). As such, I'm not sure if this is a good fix for #1951.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Mar 22, 2018

Note: The repr in the plain text mimetype isn't always nice and clean (it can become quite large for complex widgets). As such, I'm not sure if this is a good fix for #1951.

Good point. Can we try it out and see? At least it's informative of what you're missing, instead of boilerplate text that gets repetitive after a while.

@vidartf

This comment has been minimized.

Member

vidartf commented Mar 22, 2018

Can we try it out and see?

If this is only intended as a stop-gap measure until #1777 / jupyter/notebook#2980 gets fixed, then sure. My preferred long-term solution would be a short HTML entry with a link to an entry in the docs describing the possible causes of seeing the message.

Use case where the repr is longer than the current message: pythreejs, e.g. https://github.com/jovyan/pythreejs/blob/master/examples/Animation.ipynb

@mwcraig

This seems fine as a temporary workaround.

I agree with Vidar about where we want to end up eventually.

@@ -708,7 +708,6 @@ def _ipython_display_(self, **kwargs):
# http://www.iana.org/assignments/media-types/media-types.xhtml.
data = {
'text/plain': repr(self),

This comment has been minimized.

@mwcraig

mwcraig Mar 22, 2018

Contributor

If we want to ensure this never gets too big, how about repr(self)[:160] (or some other appropriate number).

This comment has been minimized.

@vidartf

vidartf Mar 22, 2018

Member

I think it is nice to have the full repr available, I'm just arguing whether that should be the fallback format or not.

This comment has been minimized.

@jasongrout

jasongrout Mar 23, 2018

Member

Since you can always do repr() to get the full repr, I think chopping it off (and appending an ellipsis) is a good compromise for the fallback.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Mar 23, 2018

My preferred long-term solution would be a short HTML entry with a link to an entry in the docs describing the possible causes of seeing the message.

+1

Chop off fallback text representation to around 110 characters.
This prevents huge text output when it is a fallback. Length 110 is chosen as it fits on a single line in the classic Notebook.

@jasongrout jasongrout force-pushed the jasongrout:removehtml branch from b4106e2 to fc173e8 Mar 23, 2018

@jasongrout

This comment has been minimized.

Member

jasongrout commented Mar 23, 2018

I just elided anything over length 110 characters - that's enough to fit on one line in the classic notebook. I think it looks all right for a fallback. What do you think?

screen shot 2018-03-23 at 12 47 27 pm

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Mar 23, 2018

LGTM

@jasongrout

This comment has been minimized.

Member

jasongrout commented Mar 23, 2018

@vidartf

This comment has been minimized.

Member

vidartf commented Mar 23, 2018

As long as we agree on the long term solution, I'm happy with the suggested solution.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Mar 23, 2018

Great, thanks!

@jasongrout jasongrout merged commit e1e0948 into jupyter-widgets:master Mar 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment