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

6 participants
@gnestor
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Sep 20, 2016

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Sep 20, 2016

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?
Contributor

ellisonbg commented Sep 20, 2016

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?
@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Sep 20, 2016

Contributor

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

Contributor

ellisonbg commented Sep 20, 2016

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

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Sep 20, 2016

Contributor
  • 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.
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Sep 21, 2016

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.

Contributor

ellisonbg commented Sep 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Sep 21, 2016

Contributor

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

@Carreau ?

Contributor

ellisonbg commented Sep 21, 2016

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

@Carreau ?

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Sep 21, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Sep 21, 2016

Member

+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.

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

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Sep 21, 2016

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Sep 21, 2016

Contributor

@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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Sep 26, 2016

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.

Contributor

ellisonbg commented Sep 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Sep 26, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Sep 28, 2016

Member

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

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

4 checks passed

codecov/patch Coverage not affected when comparing 179bb24...a0fd16c
Details
codecov/project 74.20% (+0.00%) compared to 179bb24
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gnestor gnestor modified the milestones: 6.0, 5.0 Sep 29, 2016

@gnestor gnestor deleted the gnestor:html-table-styles branch Sep 29, 2016

@gnestor gnestor modified the milestones: 5.0, 6.0 Oct 28, 2016

@mgeier

This comment has been minimized.

Show comment
Hide comment
@mgeier

mgeier Dec 22, 2016

Contributor

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?

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

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Dec 22, 2016

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!

Contributor

ellisonbg commented Dec 22, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Dec 22, 2016

Contributor

Issue on JupyterLab:

jupyterlab/jupyterlab#1436

Contributor

ellisonbg commented Dec 22, 2016

Issue on JupyterLab:

jupyterlab/jupyterlab#1436

@mgeier

This comment has been minimized.

Show comment
Hide comment
@mgeier

mgeier Dec 22, 2016

Contributor

@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.

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

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Dec 22, 2016

Contributor
Contributor

ellisonbg commented Dec 22, 2016

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

@PyAntony

This comment has been minimized.

Show comment
Hide comment
@PyAntony

PyAntony Jun 11, 2017

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

PyAntony commented Jun 11, 2017

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

@aakhmetz

This comment has been minimized.

Show comment
Hide comment
@aakhmetz

aakhmetz Jun 19, 2017

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

aakhmetz commented Jun 19, 2017

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

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Jun 22, 2017

Contributor

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 👍

Contributor

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 join this conversation on GitHub. Already have an account? Sign in to comment