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

Update table styles to be consistent with JupyterLab #1776

Merged
merged 2 commits into from
Sep 28, 2016

Conversation

gnestor
Copy link
Contributor

@gnestor gnestor commented Sep 20, 2016

Closes #1774 and #1182

Before:
image

After: image

@gnestor gnestor added this to the 5.0 milestone Sep 20, 2016
@gnestor gnestor self-assigned this Sep 20, 2016
@gnestor
Copy link
Contributor Author

gnestor commented Sep 20, 2016

By setting table-layout: auto, we get full-width table for wide-format displays (vs. a left-justified, fixed-width table) without squishing columns on narrow-format displays.

table-layout: auto:

image

table-layout: fixed:

image

@ellisonbg Thoughts?

@ellisonbg
Copy link
Contributor

Looks great - two question?

  1. What if the table wouldn't otherwise take up the full width (only a few cols). Does the auto setting stretch it to full width?
  2. What if the table has many many cols? Does this setting try to squish all of them into full width or will it start to scroll horizontally?

border-collapse: collapse;
margin: 1em 2em;
thead {
border-bottom: 2px solid @rendered_html_border_color;
Copy link
Contributor

Choose a reason for hiding this comment

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

We are using 1px here in JupyterLab

@ellisonbg
Copy link
Contributor

Also, in JupyterLab, we are using Material Design colors for everything (in particular the greys). Are we doing that here as well?

@gnestor
Copy link
Contributor Author

gnestor commented Sep 20, 2016

  • Here's an example of 2 columns and 20 columns respectively:
    image
  • notebook uses Bootstrap, so we have Bootstrap variables at our disposal. For this PR, I just used the hex for the zebra-stripe color in JupyterLab.
  • I updated the border-width for thead.

@ellisonbg
Copy link
Contributor

Hmm, the 2 cols case in wonky. Unless there is a way to limit the width, let's use the fixed approach that we did in JupyterLab.

@ellisonbg
Copy link
Contributor

Also being a pure style change, we could probably backport this to the next 4.x release...

@Carreau ?

@Carreau
Copy link
Member

Carreau commented Sep 21, 2016

Also being a pure style change, we could probably backport this to the next 4.x release...

-1 on backporting a big visual change to a minor release, even if the change is good.
I like to make the parallel : Visual Design is the API between the screen and the user brain,
and tend to favor consistency over improvement.

It's also, I think, a recipe to get questions during tutorials like "I don't have the same thing how do I get that".
And we've seen that with matplotlib 2.0 which is an easy thing to remember. Style changed on Major versions, which is easy to remember.

That's just a preference though.

@Carreau
Copy link
Member

Carreau commented Sep 21, 2016

+1 for the change otherwise,
my guess is that a well chosen ":hover" on rows can be of great help onwide screen, but that's true from JupyterLab as well.

I would also strongly recommend to read this piece of news

Q: So how do you get people [to upgrade] with urgency?
A: Change the wallpaper.
...
The theory was that [people] will say, "Whoa, this is a big deal," [...]and they will abandon their old and busted build with the horrible bug and install the new hotness.

@gnestor
Copy link
Contributor Author

gnestor commented Sep 21, 2016

Ok, I will remove that last commit and I agree about backporting.

@gnestor gnestor modified the milestones: 4.3, 5.0 Sep 21, 2016
@gnestor
Copy link
Contributor Author

gnestor commented Sep 21, 2016

@Carreau Sorry, I didn't see your comment! I understand your point about holding off until a major release... According to our release policy, UI changes are reserved for major releases. However, I am comfortable releasing this with 4.3 as it changes a non-interactive UI (vs. something interactive, like the menu or toolbar). Given the Windows blog post, we should save the shiny new table for a rainy day 😋.

Regarding :hover, here's a quick stab at it using a lighter shade of the selected cell indicator color:

screenshot

I think it's a nice feature to have whether the table is very wide or not.

@ellisonbg What are your thoughts about the hover and backporting given @Carreau's thoughts?

@ellisonbg
Copy link
Contributor

My only concern about waiting for 5.0 is that release may take a while to get out. Two reasons: i) in the past we have planned on doing that at the same time as JupyterLab 1.0 and ii) there have been Ui changes in master that will require a lot of UI/UX/design review and testing.

But in general, I do think that pushing major design changes in point releases isn't great. Would love to get @fperez take on that.

In terms of a hover, I am open to it. But let's merge this PR without it and then play with different hover styles in JupyterLab to see what makes sense.

@Carreau
Copy link
Member

Carreau commented Sep 26, 2016

My only concern about waiting for 5.0 is that release may take a while to get out [...] in the past we have planned on doing that [5.0] at the same time as JupyterLab 1.0

And that's exactly the reason I don't want to backport. There have been a number of things we said we wouldn't do (like make JupyterLab only an extension) exactly to push notebook 5.0 forward.
I also saw that the notebook_server was forked from notebook which IMHO is too early, and one more reason to neglect the current notebook.

Backporting starts to become hard, and I'm in favor of saying that if you want things out, then some cycles of the developers of JupyterLab should be spent on making 5.0 a reality. I don't have much time to help @gnestor and I don't want to bother him more. PLus with 4.x growing and 5.0 on it's way + JupyterLab, the testing of each branch by developer is quasi-null. Which increase the risk of bugs.

Also, for example 4.x does not have the ability to order file by date in the dashboard which is well beyond time to release. So while I can't prevent you from backporting things to 4.x, I'm exceptionally going to start putting the strongest -1 on backporting to 4.x until 5.0 is out, starting with this.

In terms of a hover, I am open to it. But let's merge this PR without it and then play with different hover styles in JupyterLab to see what makes sense.

Agreed.

@Carreau Carreau modified the milestones: 5.0, 4.3 Sep 28, 2016
@Carreau
Copy link
Member

Carreau commented Sep 28, 2016

Ok, merging this, if we decide to backport it we can still do it later.

@Carreau Carreau merged commit 111e41f into jupyter:master Sep 28, 2016
@gnestor gnestor modified the milestones: 6.0, 5.0 Sep 29, 2016
@mgeier
Copy link
Contributor

mgeier commented Dec 22, 2016

I like the new table style, but is the max-width setting really right?

I have a notebook with an innocuous table in it:

field name | contents
-----------|---------
left | impulse responses for the left ear
right | impulse responses for the right ear
fs | sampling rate
apparent_azimuth | directions of sound incidence (in radians)

For comparison, this is how it is rendered right here on Github:

field name contents
left impulse responses for the left ear
right impulse responses for the right ear
fs sampling rate
apparent_azimuth directions of sound incidence (in radians)

With the new table style, it looks like this:

truncated

I don't see a reason for limiting the column width in this case. Without the max-width setting, it looks like this:

full-width

I don't think my cell contents are exceedingly wide.
I understand that in some cases the column width has to be limited, but I think in this case it's not the right thing to do. If anything, there should be line breaks, but making part of the content invisible seems strange ...

Should the width probably be limited in auto-generated tables (like pandas data frames) and not limited in handcrafted tables in Markdown cells?

@ellisonbg
Copy link
Contributor

@mgeier the issue is that in the wild it is very common to have tables that have dozens or even hundreds of columns. When that shows up, not having the max-width is really painful.

However, you do make a really good point about tables in markdown. Those are handcrafted and would usually be smaller. I would be fine if we add some conditional CSS to remove the max-width on those. Can you submit a PR or at least open an issue targeted at the 5.0 milestone? Thanks!

@ellisonbg
Copy link
Contributor

Issue on JupyterLab:

jupyterlab/jupyterlab#1436

@mgeier
Copy link
Contributor

mgeier commented Dec 22, 2016

@ellisonbg Done: #2008

I can't set a target milestone, I've just mentioned it in the text.

Sorry I can't actually implement this, I have too much in my pipeline already.

@ellisonbg
Copy link
Contributor

ellisonbg commented Dec 22, 2016 via email

@gnestor gnestor moved this from Open PRs to Merged PRs in 5.0 Feb 4, 2017
@PyAntony
Copy link

How can I display tables with the old style? There must be a way to do it...

@aakhmetz
Copy link

something has changed, the notebook just became slower for me and less convenient with this new style.

@gnestor
Copy link
Contributor Author

gnestor commented Jun 22, 2017

The changes were all made to https://github.com/jupyter/notebook/blob/master/notebook/static/notebook/less/renderedhtml.less in #1776. To switch back to old styles, you can manually edit this file or if you're running from a clone of this repo, just revert the commit in #1776.

We are open to suggestions as to how to improve the table style 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
5.0
Merged PRs
Development

Successfully merging this pull request may close these issues.

Side-port HTML table redesign from JupyterLab
6 participants