Refactor stylesheets + update typography #101

hoetmaaiers · 2020-02-26T22:07:54Z

Refactor theme.scss + _sphinx-bootstrap.scss into single stylesheet.
Update typography.

This fixes the yarn build:production script

src/scss/theme.scss

choldgraf · 2020-02-26T22:19:24Z

I think it's a good idea 👍 though see my comment in #100 about using child files for scss and importing them into a master SCSS file

choldgraf · 2020-02-27T18:16:00Z

It looks nice to me - I'd be +1 on merging and then iterating on things moving forward.

A couple quick notes:

Because the typography is smaller and there is more space, the whitespace on the right feels awkward to me
When the right TOC is long enough to be scrollable, the scroll bar is super wide relative to other elements of the page, perhaps we can make it thinner? The same is true for the left sidebar. I think this is now more noticeable because the content isn't centered, so it could be that this would go away w/ centered content

jorisvandenbossche · 2020-02-27T18:37:47Z

@hoetmaaiers thanks for getting this started!

In general I think it is a really nice improvement (better typography / vertical spacing). Some specific comments on how it looks for the demo site:

Personally, I find the font too small (at least on my laptop, but I have this for other sites often as well). It might also be the combination of the font and font size.
I find the width of the main content a bit too small (even with the small font). So it seems it now uses 600px, while eg readthedocs uses 800px which I personally already find not very wide (docker for example uses 1024px). Of course the exact width of the text also depends on the margin width, so the numbers are not directly comparable.
One concrete example of the narrow width is that I think a normal line of python code in a code block of 79 characters should easily fit (while now it gives a horizontal scrollbar)

hoetmaaiers · 2020-02-27T21:54:15Z

@choldgraf, do you refer to the whitespace between body and TOC?

@jorisvandenbossche , good point about the max-width of the content. The 600px width sneaked into my refactored stylesheet code tbh. Opting for a max-width of 1024px would be my preference too.

--

I updated this PR with a larger base font-size, where 1em == 15px. Also the body content will have a max width of 1024px.

The specific white space, font-sizes, element widths, ... would be something that I want to tackle and discuss in the full-width work #17. This will be a rework of the existing code and therefore a good opportunity to overhaul these things. What do you think?

choldgraf · 2020-02-28T03:07:54Z

@hoetmaaiers thanks for the update. For your question, I meant the whitespace to the right of the TOC (so the whitespace at the far-right of the page). As @jorisvandenbossche mentions, I feel like on a medium-sized laptop, the whole screen should be taken up (there should be no horizontal white space)

I don't see any recent pushes to this PR, can you double-check that it is updated?

hoetmaaiers · 2020-02-28T07:50:24Z

Indeed, I even forgot the commit step 😴. Now it should be here (though minor).

choldgraf · 2020-02-28T16:54:12Z

Hmm - this is how the preview looks on my 13" monitor:

I think we should either widen the sidebars, or widen the main content, to fill up a 13" screen at the least. Maybe we could also use some whitespace to have as a sidebar. Sphinx has a sidebar directive that might be useful for adding contextual information in the docs...

jorisvandenbossche · 2020-02-28T17:48:22Z

Hmm, the latest build version of the demo site doesn't seem to have picked up the font size / width change. It is still using the 600px width.

hoetmaaiers · 2020-02-28T21:12:28Z

Now it has. I forgot the step yarn build:production. This is a tricky one to forget, maybe a precommit hook can solve this? Only unsure if we want to enforce it this way...

choldgraf · 2020-02-28T21:16:33Z

Ah - looks much better now. I'm +1 on these typography updates, and I'm assuming that the CSS refactor isn't changing the look of anything?

Just a note - things like "I forgot yarn build:production before committing" are precisely the kinds of things I am worried about with respect to using webpack. These steps add cognitive burden to developers, and especially ones that do not do web development (which is 90% of Python developers) they won't be intuitive. We should definitely find a way to avoid devs needing to remember this

hoetmaaiers · 2020-02-28T21:29:32Z

I absolutely agree on the cognitive burden which will cause too much trouble when forgotten.

Shouldn't we automate this? Ideas:

Git precommit hook which will run yarn build:production. We must ensure this is and remains a light enough operation.
Include in the Circle CI automated steps. Unsure if this is the correct place for it.
Print out clearly after committing something like "Make sure to run yarn build:production to commit theme changes."

choldgraf · 2020-02-29T01:13:54Z

@hoetmaaiers maybe something to open a new issue about? I think it's a good idea to try and design around the confusion as much as possible (though we are also sort-of trading one cognitive overhead for another :-) )

jorisvandenbossche · 2020-02-29T13:44:19Z

Yep, I agree we need to add in place checks to ensure this doesn't happen (or get merged), opened #104

jorisvandenbossche · 2020-02-29T13:45:55Z

docs/demo/generated/pandas.DataFrame.drop.rst

+
+.. currentmodule:: pandas
+
+.. automethod:: DataFrame.drop


It seems you accidentally added those to the tracked files?
(we might need to add this directory to gitignore if that's not yet the case)

Is is not yet part of gitignore. Should docs/demo/generated be added as a whole?

Yes, this full /generated/ directory can be added to gitignore

jorisvandenbossche · 2020-02-29T13:55:35Z

src/scss/theme.scss

+  margin-bottom: 1.15rem;
+
+  // &.rubric {
+  //   border-bottom: 1px solid rgb(201, 201, 201);


This is no longer needed?
You can check it at https://144-130237506-gh.circle-artifacts.com/0/html/demo/generated/pandas.DataFrame.drop.html#pandas.DataFrame.drop, where indeed there is still a line under "Examples". But it is strange that this still seems to come from sphinx-bootstrap.css, which should be removed now ..

@hoetmaaiers so there still is pandas_sphinx_theme/static/sphinx-bootstrap.css_t that should be removed I think? (since you moved that content into /src/, otherwise it duplicates the css)
That's also the reason that I still see a correct rendering of the "Examples" title (see my comment above). So the "rubric" css above probably needs to be uncommented.

Well spot, this is fixed.

I was also thinking of using a welback clean plugin. This removes the generated output in /static. This makes it more clear to the committing person that yarn build:production should be ran or all js & css output is deleted in the commit.

jorisvandenbossche

In the demo site, I just noticed that the right sidebar TOC is no longer collapsed (right now in master, it only expands that section where you are while scrolling, as otherwise this TOC can be very long (at least in pandas docs))

jorisvandenbossche · 2020-02-29T13:58:18Z

The width now seems fine! (and again this starts to look nice ;))

Personally, I still find the overall text size a bit small, but will check tomorrow on a desktop screen (now on my laptop, which also for other sites is often a bit small I find).

jorisvandenbossche · 2020-02-29T13:59:12Z

Personally, I still find the overall text size a bit small,

Also, maybe the font size for the items in the top bar can be a bit larger? (but also fine to keep fine tuning several navbars in another PR)

choldgraf · 2020-02-29T16:29:35Z

I'd be +1 on merging this one since it's also a bit of a refactor, and then spot-checking improvements in the future. WDYT @jorisvandenbossche ?

jorisvandenbossche · 2020-02-29T21:47:44Z

Yes, I am certainly fine with merging this mostly as is, leaving further style improvements for a next PR. There are only a few thinks that needs to be fixed first (my inline comments).

src/scss/theme.scss

jorisvandenbossche · 2020-03-02T08:49:02Z

src/scss/theme.scss

+
+  td:first-child {
+    white-space: nowrap;
+  }


Now this seems to be nested inside "table.field-list", while this was originally not the case?

You can see the result of that at https://146-130237506-gh.circle-artifacts.com/0/html/demo/api.html#autosummary-table-and-api-stub-pages (compare with the table at https://pandas-sphinx-theme.readthedocs.io/en/latest/demo/api.html#autosummary-table-and-api-stub-pages).

(that's also a reason to preserve the original comments, as that was trying to explain this is specifically for "autosummary tables")

Ok, good remark. Deleted too eagerly.

jorisvandenbossche · 2020-03-02T08:49:08Z

src/scss/_sphinx-bootstrap.scss

-
-/**
- * Styling for field lists
- */


Can you keep the comments that were here originally in the new scss file?

Ok, brought that back.

…stylesheets

jorisvandenbossche · 2020-03-04T16:54:59Z

Just added back a few more comments ;)
But so now the demo site looks all good (the scrolling works fine again), so let's merge this! And further style improvements can go in separate PRs.

choldgraf · 2020-03-04T17:07:41Z

wohoo! thanks for all your hard work on this one @hoetmaaiers :-)

hoetmaaiers · 2020-03-04T19:10:01Z

Yay, thank you for the comments @jorisvandenbossche, these will come in handy when refactoring with the full width rework.

hoetmaaiers added 3 commits February 26, 2020 22:54

Add webpack for static asset generatino

d801069

Add optimize-css-assets-webpack-plugin

677f398

This fixes the yarn build:production script

Refactor _sphinx-bootstrap.scss into theme stylesheet

f2bfd1e

hoetmaaiers mentioned this pull request Feb 26, 2020

Bootstrap is being loaded twice #83

Closed

Merge branch 'master' into feature/refactor-stylesheets

8ce4ae9

choldgraf reviewed Feb 26, 2020

View reviewed changes

src/scss/theme.scss Show resolved Hide resolved

Update base font-size and body max-width

b74ae31

hoetmaaiers force-pushed the feature/refactor-stylesheets branch from 2c65a21 to b74ae31 Compare February 28, 2020 21:11

jorisvandenbossche reviewed Feb 29, 2020

View reviewed changes

hoetmaaiers added 2 commits March 1, 2020 11:42

Remove sphinx-bootstrap.css_t, include CleanWebpackPlugin

0aeae9f

Add "docs/demo/generated" to gitignore

a75b2af

jorisvandenbossche reviewed Mar 2, 2020

View reviewed changes

src/scss/theme.scss Outdated Show resolved Hide resolved

src/scss/theme.scss Outdated Show resolved Hide resolved

jorisvandenbossche reviewed Mar 2, 2020

View reviewed changes

Bring back comments and css logic from previous stylesheet

2aefc58

hoetmaaiers requested a review from jorisvandenbossche March 3, 2020 21:58

jorisvandenbossche changed the title ~~Feature/refactor stylesheets~~ Refactor stylesheets + update typography Mar 4, 2020

jorisvandenbossche added 3 commits March 4, 2020 17:49

Merge remote-tracking branch 'upstream/master' into feature/refactor-…

3170a26

…stylesheets

Merge remote-tracking branch 'upstream/master' into feature/refactor-…

1201f67

…stylesheets

add more comments

3e0684c

jorisvandenbossche approved these changes Mar 4, 2020

View reviewed changes

jorisvandenbossche merged commit 14085ba into pydata:master Mar 4, 2020

jorisvandenbossche mentioned this pull request Mar 23, 2020

ENH: max-width layout for wide screens #134

Merged

hoetmaaiers deleted the feature/refactor-stylesheets branch April 3, 2020 06:44

pradyunsg mentioned this pull request Sep 9, 2020

Thoughts on dropping Lato, and using the system/native font stack instead? #247

Closed

Refactor stylesheets + update typography #101

Refactor stylesheets + update typography #101

Uh oh!

Conversation

hoetmaaiers commented Feb 26, 2020

Uh oh!

Uh oh!

choldgraf commented Feb 26, 2020

Uh oh!

choldgraf commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorisvandenbossche commented Feb 27, 2020

Uh oh!

hoetmaaiers commented Feb 27, 2020

Uh oh!

choldgraf commented Feb 28, 2020

Uh oh!

hoetmaaiers commented Feb 28, 2020

Uh oh!

choldgraf commented Feb 28, 2020

Uh oh!

jorisvandenbossche commented Feb 28, 2020

Uh oh!

hoetmaaiers commented Feb 28, 2020

Uh oh!

choldgraf commented Feb 28, 2020

Uh oh!

hoetmaaiers commented Feb 28, 2020

Uh oh!

choldgraf commented Feb 29, 2020

Uh oh!

jorisvandenbossche commented Feb 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche commented Feb 29, 2020

Uh oh!

jorisvandenbossche commented Feb 29, 2020

Uh oh!

choldgraf commented Feb 29, 2020

Uh oh!

jorisvandenbossche commented Feb 29, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche commented Mar 4, 2020

Uh oh!

choldgraf commented Mar 4, 2020

Uh oh!

hoetmaaiers commented Mar 4, 2020

Uh oh!

Uh oh!

choldgraf commented Feb 27, 2020 •

edited

Loading