ENH: max-width layout for wide screens

[hoetmaaiers]

This PR adds a max-width layout as discussed in #17.

I would like a first round of feedback before continuing on extra related work:

• Refactor scss more and make use of partials
• Suggestion, remove explicit "Home" link? The brand logo has this functionality, which is also a pattern universally applied on the web?

[jorisvandenbossche]

This looks nice! Didn't look at any code, just the rendered docs (https://212-130237506-gh.circle-artifacts.com/0/html/demo/example_pandas.html), and some quick first comments:

• Similarly as I originally mentioned in Refactor stylesheets + update typography #101 (comment) as well, I find the main content width a bit too small (or at least on my large screen when there is a lot of space, see the screenshot below). I currently get a width of only 665px.
• The TOC left sidebar is also made narrower? At least when there is space, that should also be allowed to take more room I think. That would avoid that longer section headers are quickly wrapped over multiple lines

For example, in the screenshot above, I would say that if there is more space, the left TOC can also use more space. I agree that if horizontal screen space is limited, the TOC sidebar is the first thing that can be limited / removed, but it would be nice to still allow it to be bigger if possible.

[stijnvanhoey]

Nice work, I really like the adjustments. I agree that a Home item in the menu is maybe not required. Just did a very quick check and noticed an issue with the width of code blocks:

or giant tables

[choldgraf]

+1 on removing the "home" link (actually, I think that we should optionally have a site title instead of a logo, and have that link to "index", and remove the home page)

the CSS/layout looks quite nice! I'm a little bit confused about the amount of code files changed - when I was playing around with centering the content, all it took was wrapping the top nav in a container, and then adding a max-width to the container class. Could you explain a bit more what's happening under the hood?

[jorisvandenbossche]

Yes, also +1 on removing the Home link and having the logo for this (although that can maybe done in a separate PR?). And being able to use text instead of a logo also sounds good as an option.

I also verified in the demo docs that the buggy behaviour we ran into before (#118) doesn't happen with this PR for me.

[hoetmaaiers]

Ok, thank you all for the feedback. I will continue my work on this and ensure the content should grow wider on a wider viewport. Also a fix for 'giant' content is in the making, well spotted @stijnvanhoey . For now I am opting for an inline scroll solution. Tables can be made responsive, but this is not straightforward.

Good question from you @choldgraf, the basic implementation would indeed be what you described. I took it a little further into detail, so this PR zooms in into some details to look more like the mockup. Also some refactoring of the css is included (I will continue this some further).

Thanks @jorisvandenbossche for testing #118.

To be continued...

[jorisvandenbossche]

Sounds good!

Small general point of feedback: to the extent possible, try to prefer smaller PRs / break certain aspects that can be done independently into its own PR. Although initially a bit more work for you (to split it up, to update branches if something else is merged ..), it will also speed up reviewing and thus allow that some things already get merged faster. To the extent that it doesn't make working on it too cumbersome of course!

(because I assume that part of the reason for the big diff that @choldgraf asked about is also because there are some (useful!) changes not strictly related to the max-width setting)

[choldgraf]

agreed - for example, I quite like the new searchbar CSS that I noticed you added :-)

[hoetmaaiers]

Good remark on keeping PR's smaller. I already tried to focus but extra's creeped in.
So my updates for this PR include:

An extra breakpoint for larger screens is added. At min width of 1500px, the container is spread out to 1400px. Which allows the body container to be 750px (excl. white space) which is still good for readability.

The bd-content allows elements wider then the bd-content container to overflow via scroll. This might be solved more elegantly. I miss the indication that content is hidden behind scroll, maybe a visual indicator (shadow) could solve this?

Home nav item removed.

[choldgraf]

I think it looks really nice 👍

one question: how do folks feel about the white space here:

Not to beat a dead horse, but many pages have lots of top-level toc items that could benefit from that extra white space :-)

[hoetmaaiers]

Some extra insight for this choice... I had the same idea and made the brand-logo and sidebar 2 columns wide. But then the body content became too wide (in my opinion). The idea of a max-width is to improve readability of the main content.

Also having lots of top-level toc items still have space issues on a smaller viewport.

[choldgraf]

couldn't the top bar have widths independent of the content beneath it?

[hoetmaaiers]

It sure can. Personally I like the symmetry more. But this is also an option:

[choldgraf]

hmmm, I like the symmetry as well - but maybe this can be breakpoint-controlled? Either way, I don't think that it should block this PR...we can always see if other folks complain about the lack of width.

[jorisvandenbossche]

The bd-content allows elements wider then the bd-content container to overflow via scroll. This might be solved more elegantly. I miss the indication that content is hidden behind scroll, maybe a visual indicator (shadow) could solve this?

Something that I noticed about this, eg on the "Lists & Tables" demo page (https://220-130237506-gh.circle-artifacts.com/0/html/demo/lists_tables.html#giant-tables), the tables now are "scrollable" (they are no longer put on top of the sidebar), but the scrollbar is not inline at the table, but only at the bottom of the full page:

hmmm, I like the symmetry as well - but maybe this can be breakpoint-controlled? Either way, I don't think that it should block this PR...we can always see if other folks complain about the lack of width.

Yes, I think we can look into different options (or ways to customize this for users) in a follow-up.
I also like the symmetry for now.

[jorisvandenbossche]

@hoetmaaiers and thanks for the updates! The default width on my screen for the content looks better now.

I have some more comments about the widths of the different columns. But not all have necessarily to be handled in this PR. And also, I am just looking at it as a user of the docs, and some of the things I am asking might not be at all straightforward, no idea!

1. I still find the width of the TOC a bit small. Consider the follow screenshot. The first title of the TOC (which is in bold), "Different choices for indexing", is now wrapped on two lines. While there is plenty of empty space to the right

2. On my full screen, the main text is now indeed 750px wide. On the screenshot below, I reduced my browser window width from 1900 to 1350px, and then the main content it is only 600px wide (which I personally find too narrow, if there is more space available). But at the same time, there is still non-utilized white space on both sides.
   I am wondering if it is possible, at some point when the screen becomes narrower, to "just" use the full width?

[jorisvandenbossche]

One more thing that I noticed on the smartphone mode:

Master (https://pydata-sphinx-theme.readthedocs.io/en/latest/):

versus this PR -> the logo is smaller (I suppose something with the allowed height changed)

Now, this might also be due to the pandas logo, as I noticed elsewhere that it has a lot of whitespace around it in the logo file itself, which makes it more difficult to let look larger in general.

[jorisvandenbossche]

This includes all of bootstrap css in our index.css, right?
I don't know if this is best to solve #89, but a downside of this is that on the actual site, this makes it difficult to see which parts of css is coming from main bootstrap, and which are customizations from this theme.

[choldgraf]

Couldn't we just bundle bootstrap, minified, in this theme, and directly link it? I'd be +1 on that, I actually need to do this in a downstream theme anyway in order to get PDF printing to work

[hoetmaaiers]

You are correct, this includes all of bootstrap in our index.css. Bundling bootstrap as in this theme would make us lose the configuration options Bootstrap provides.

A solution to might satisfy both would be to have 2 css outputs defined in Webpack.

1. Full bootstrap with custom configuration applied.
2. Custom css applied after bootstrap.

Would this be a solution for @jorisvandenbossche and @choldgraf cases?

This should be a different PR, since it might be more complicated then it seems. Especially in combination with #139 to allow bootstrap Mixins to be available in the custom css without bundling twice.

[choldgraf]

I actually found a workaround on my end that didn't require bundling bootstrap, though I think there's still a benefit to this for working in an offline context. Depending on how complex it makes things I am interested!

[jorisvandenbossche]

I think that at least sounds an interesting option to explore, to see if that would be possible to have two css outputs.
But indeed fine to do as a follow-up

[choldgraf]

@jorisvandenbossche re: your comment about overflows etc, doing #131 seemed to fix it for me (particularly the "overflow" bootstrap classes)

[hoetmaaiers]

Ok, most issues are fixed in this PR. A summary:

• Scrolling large content is solved by allowing a .section element overflow with scrolling behaviour. I chose .section on purpose because it will group related content together and also scroll this related content as a whole. Thank you for the alternative suggested solution @choldgraf .

• The TOC sidebar now takes more advantage of the whitespace from a certain breakpoint. This is also an initial use of the bootstrap provided breakpoint mixins: https://github.com/pandas-dev/pydata-sphinx-theme/pull/134/files?file-filters%5B%5D=.css&file-filters%5B%5D=.js&file-filters%5B%5D=.lock&file-filters%5B%5D=.scss#diff-e3d1d830790a12807fb397358c25d549R170

• The pandas logo sizing is solved. Sorry for the bug, should have catches this one.

Things I would move into separate PRs:

• Making better use of full sized screens. I have an elegant solution ready as POC, but should test it on more thoroughly.
• Split up bootstrap and custom css.

[choldgraf]

so this is another example that could be a breakpoint include right?

[hoetmaaiers]

Yep!

[choldgraf]

I think it looks nice 👍 I'd be +1 on merging this as a clear improvement, and then spot-checking things in the future

[jorisvandenbossche]

OK, sounds good, let's merge this now, and handle remaining feedback in follow-up PRs.

Thanks @hoetmaaiers !