Skip to content

DOC: Makeover for Sphinx pages #17024

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

Closed
wants to merge 6 commits into from
Closed

DOC: Makeover for Sphinx pages #17024

wants to merge 6 commits into from

Conversation

bjnath
Copy link
Contributor

@bjnath bjnath commented Aug 6, 2020

Larger type, clearer sidebars, and a more modern look, extending the #16988 update.

New:

image

Previous:

image

Looking forward to comments (@seiko2plus, dig in!).

This is again a change just to layout.html. I tried linking to a numpy.css stylesheet in a static directory, but I found that when the pages were built my relative link to the stylesheet did not always end up pointing to the same path, for reasons I can't explain.

I'll post a link to the CircleCI build object once the build completes.

bjnath added 2 commits August 6, 2020 13:33
Extending the makeover of the Sphinx pages for larger type, clearer
sidebars, and a more modern look.
@charris charris changed the title Makeover for Sphinx pages DOC: Makeover for Sphinx pages Aug 6, 2020
@bjnath
Copy link
Contributor Author

bjnath commented Aug 7, 2020

@rgommers
Copy link
Member

rgommers commented Aug 7, 2020

Ooh very nice:) Thanks @bjnath, this makes a big difference - looks much more 2020 rather than 2010.

If you're up for another tweak, the yellow in the signature boxes could use an update (not sure what, maybe some blue-ish color in the website color palette?):

image

@bjnath
Copy link
Contributor Author

bjnath commented Aug 7, 2020

@rgommers --

the yellow in the signature boxes

It's a highlight (doesn't appear if you click on the page directly); a blue wouldn't have made clear it's a highlight -- changed to a diluted version of the mustard in the palette:

image

Not quite so garish.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 7, 2020

@rgommers -- Or we can just turn highlighting off.

@eric-wieser
Copy link
Member

Highlighting is useful when coming from a search term, or if there are multiple functions on one page. Can we just disable it for single-function autosummary pages?

@bjnath
Copy link
Contributor Author

bjnath commented Aug 7, 2020

disable it for single-function autosummary pages?

That would be logical, but it'd take deeper thinking than I'm up for. I agree, highlighting is useful, and perhaps the color in #17024 (comment) is a decent compromise.

layout.html now reads a static numpy.css file, thanks to @eric-wieser.

At @eric-wieser's suggestion, removed percentage spacing dimensions, added
formatting cleanup.

Per @rgommers, made highlighting color less garish.
@eric-wieser
Copy link
Member

My comment was more a disagreement against total removal. Muting the color strikes me as fine.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 7, 2020

Incorporated changes -- thanks @eric-wieser for your suggestions, particularly the stylesheet save, and @rgommers for your suggestion on highlights.

The bigger titles mean that the names of some routines in the API pages extend off the page; in those cases a horizontal scrollbar appears:
image

I'll post the new CircleCI link when it comes around.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 7, 2020

Copy link
Member

@seiko2plus seiko2plus left a comment

Choose a reason for hiding this comment

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

#[1/4] Increase the responsivity and expanding the width.
Reasonable due to increasing font-size and shfiting the main headline to the other side

font-size: 16px;
line-height: 150%;
}

This comment was marked as resolved.

dt:target, span.highlighted {
background-color: #fff7e6; /* diluted mustard */
}

This comment was marked as resolved.


div.top-scipy-org-logo-header {
background-color: #fafafa;
border-bottom: 2px solid #013243; /* warm black */

This comment was marked as resolved.

Logo section fills page, per @seiko2pro.
Uniform CSS indenting, per @eric-wieser.
@bjnath
Copy link
Contributor Author

bjnath commented Aug 8, 2020

@eric-wieser: Corrected indents.

@seiko2plus: Increased logo section to fill page; thanks -- much better. Still reluctant to widen the text, even at the increased type size. It's just readable now and I like the white borders.

Wide version:
image

Committed version:

image

I increased the logo size a little, which I'm already regretting and am changing back.

At 125px it competes with the heading; restored to the earlier 110px.
@bjnath
Copy link
Contributor Author

bjnath commented Aug 8, 2020

@eric-wieser
Copy link
Member

Agreed that the full width is worse - especially on wide monitors

display: none;
}

/* Don't show the page title in TOC */
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to remove this from the template, rather than just hiding it in the CDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two elements I've hidden are the unnecessary "Table of Contents" title and the unnecessary top link that's the page name; what template change would do this instead?

Copy link
Member

Choose a reason for hiding this comment

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

I've no idea, just thought it might give you more control.

Do you know which template emits the sidebar?

Also, would these changes make more sense in the numpy Sphinx theme, wherever that lives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want to remove this from the template?

I understand you now. I think the partitioning is OK -- all the style changes are in one place, the numpy.css file, and the only template change is to point to that file.

would these changes make more sense in the numpy Sphinx theme, wherever that lives

My first thought was to do that, but I think we're just a veneer on the scipy theme, which we have as a submodule.

Copy link
Member

Choose a reason for hiding this comment

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

but I think we're just a veneer on the scipy theme, which we have as a submodule.

Yes, I was thinking of the abandoned PR below.

If scipy wants similar changes (@rgommers?), then perhaps you should edit the theme directly.

Copy link
Member

Choose a reason for hiding this comment

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

I think there are much better Sphinx themes around (e.g. the statsmodels and pandas ones), so longer term I do think we want to get rid of the current theme and just adopt another one. The maintenance cost of a theme is quite high, so sharing the load will be helpful too.

I'm happy with the current approach, adding a numpy.css. With the main goal being to improve the most glaring issues (font size, and styling which matches the website better). Other improvements are nice to have, but no need to redesign half the theme I think, because at some point we'll want to get rid of it.

@seiko2plus
Copy link
Member

seiko2plus commented Aug 8, 2020

What I suggested is a full width with max-width 1500px and stretching the reading sections to the max.

The current:

current

The suggestion:

mine

@eric-wieser
Copy link
Member

This likely conflicts with #14648. Are we considering that PR abandoned?

@rgommers
Copy link
Member

rgommers commented Aug 8, 2020

This likely conflicts with #14648. Are we considering that PR abandoned?

Yes I think so.

@eric-wieser
Copy link
Member

eric-wieser commented Aug 8, 2020

@rgommers: So by association, the https://github.com/scipy/scipy-sphinx-theme-v2 repo is defunct?

Incorporates @seiko2plus's changes so
sidebar moves with main text when page is widened.
@bjnath
Copy link
Contributor Author

bjnath commented Aug 8, 2020

@seiko2plus Sorry I didn't see it right away -- your styling suggestions are a huge improvement, once again, and I put them in. Our one difference is max width, which I've set at 900px rather than 1500px.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 8, 2020

@seiko2plus
Copy link
Member

@bjnath, 900px is kinda tight, I feel like 1100px gonna make everybody happy try it locally at least.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 8, 2020

I've linked full-size page images at widths of 900, 1000, and 1100 pixels, so everyone can take a look.

Clicking the link shows a long, skinny image; then click on the image to see the page life-size.

900px

1000px

1100px

Here's an inconclusive link on recommended line lengths.

Character counts of the first line of these pages:

900px  78
1000px 88
1100px 94

@seiko2plus
Copy link
Member

@bjnath, My concern is to expand figures and code samples on wide screens.
Not sure to be honest about what the line length should be but I daily use Wikipedia
and the line there can normally reach 180-200 chars.
Also on the other side, the user can always turn on the reader view from the browser.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 9, 2020

@seiko2plus I've noticed that about Wikipedia!

I don't want habits to keep me from seeing a better idea -- let's let this settle for a bit and see if others want to say something.

Figures and code samples is something I hadn't considered, but I wasn't sure I understood. Code samples should not get too wide because they follow coding-style line-length restrictions. For figures, we probably don't want figures that don't show up well unless the page is expanded. I imagine tables is another example in this genre, but I don't think NumPy docs ordinarily need to display tables with many columns. Did I misunderstand?

Fortunately max width is an easy changeover. It can even be done later if we think we've chosen wrong.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 9, 2020

Looking at the CI build, I see NEPs were not restyled by this change -- they have just the #16988 changes.

I'm not clear why that is -- I'd expect they'd either have this change or none.

Their appearance seems OK as-is; they're clearly a separate section, and they look fine with the #16988 changes.

@eric-wieser
Copy link
Member

I think we should leave line lengths for another PR, rather than piling on increasingly subjective changes in a single PR. My main concern now is whether scipy wants these changes too, and if so if we should just make them directly at scipy/scipy-sphinx-theme

Copy link
Member

@seiko2plus seiko2plus left a comment

Choose a reason for hiding this comment

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

Alright, we can just avoid talking about line width for now.
#[2/4] These changes aim to modernize the navbar including breadcrumbs and nav-buttons,
the reason behind them is to clarify the sidebar view (table of contents),
so the ideas can be flowing to our minds(or at least my mind) since honestly, I don't like how the sidebar looks right now.

Before
before

After
after

Comment on lines +4 to +8
.main {
-moz-box-shadow: none;
-webkit-box-shadow: none;
box-shadow: none;
}

This comment was marked as off-topic.

Comment on lines +20 to +29
.container > div {
max-width: 900px;
margin: 0 auto;
}

/* Expand header bg*/
.container:first-child {
background-color: #fafafa;
border-bottom: 2px solid #013243; /* warm black */
}

This comment was marked as off-topic.

Comment on lines +42 to +44
div.spc-navbar .nav-pills > li > a {
background-color: #4d77cf; /* han blue */
}

This comment was marked as off-topic.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 9, 2020

make them directly at scipy/scipy-sphinx-theme

Let's not be doctrinaire. The PR took me a day to write only because I knew nothing about the scipy theme. If I'd started by trying to understand the theme and making changes bottom-up we'd have pieces all over the floor and still no PR.

Heed the lessons of #14648.

piling on increasingly subjective changes in a single PR

To be clear, nothing's being piled -- it's the value of one number in one line of the stylesheet.

And increasingly subjective? It's not as if we've been trying on clothes for a week. It's the one esthetic decision we've discussed; @seiko2plus has earned a respectful hearing, and presumably if he feels strongly, and I feel strongly, others also feel strongly; so let's give everyone a moment to speak up. I've provided links to look over the choices.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 9, 2020

piling on increasingly subjective changes

I had not seen the most recent proposals. Yes, @seiko2plus, you've made impressive changes as usual, but it's big and I'm not sure I'm on board yet; maybe it should go in a subsequent PR so we can do it justice.

@seiko2plus
Copy link
Member

seiko2plus commented Aug 9, 2020

@bjnath, I thought I'm here to review the design, not just the code. The layout parts are linked to each other so any real suggestions will lead to massive changes and that looks normal to me.
However, feel free to not accept the requested changes but in that case, I'm not gonna able to review/suggest changes
that related to the design.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 9, 2020

I wrote the PR to have something better than what we had. There was no attempt to establish a new world order or solve everything we hoped to solve. And I'm not sure the reviewers are expecting that either right now.

Everything you've suggested to this point has been a slam-dunk obvious improvement. This latest is a new conception and a new kind of discussion.

Let's agree that what's here now is not everything we hope for, but that it can stand on its own while we plot what else needs doing. I expect that discussion will continue for a while.

@seiko2plus
Copy link
Member

seiko2plus commented Aug 9, 2020

Everything you've suggested to this point has been a slam-dunk obvious improvement. This latest is a new conception and a new kind of discussion.

The lastest suggested changes looks obvious to me because I don't like the new sidebar changes and at the same time
I couldn't suggest new changes to the design without improving the navbar since the navbar comes first.

Let's agree that what's here now is not everything we hope for, but that it can stand on its own while we plot what else needs doing.

Oki, please feel free to not accept the requested changes, and I will move them later to a new pull combined with the other undiscovered suggestions that related to the sidebar.

I expect that discussion will continue for a while.

No no, please I don't want to be an obstacle here. I was just trying to fill the gap, sorry :).

@bjnath
Copy link
Contributor Author

bjnath commented Aug 9, 2020

I was just trying to fill the gap, sorry :)

I'm very grateful for your help.

Eager to see what you have in mind for the sidebar.

@eric-wieser
Copy link
Member

eric-wieser commented Aug 10, 2020

If I'd started by trying to understand the theme and making changes bottom-up we'd have pieces all over the floor and still no PR.

My understanding is you can just drop your stylesheet changes in that repo (https://github.com/scipy/scipy-sphinx-theme/blob/master/_theme/scipy/static/scipy.css_t). The mistake in #14648 seemed to be starting from scratch without any real buy-in for doing so.

@rgommers
Copy link
Member

@rgommers: So by association, the https://github.com/scipy/scipy-sphinx-theme-v2 repo is defunct?

Yes, I don't think it's going to make it over the finish line:( As discussed in the last docs meeting, I think the scope of what @bjnath is doing here should be limited to making the style match the website a little better and fixing the most egregious issues. Eventually I think we would like a bigger makeover, but at this point I'd much rather adopt a good and well-maintained theme (e.g. one of the Pandas or Statsmodels ones) than keeping our own.

@bjnath
Copy link
Contributor Author

bjnath commented Sep 27, 2020

Closing because we now use a new theme (#17074)

Some of these changes have been migrated to that theme (#17382).

@bjnath bjnath closed this Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants