Skip to content

Adds wrapping to tables in docs for improved readability. #1729

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 1 commit into from

Conversation

typesanitizer
Copy link

I've also added a dependency on sphinx_rtd_theme in the environment.yml file as
the docs wouldn't build without it.

(I've ticked the last checkbox but I'm not sure if I'm supposed to document anything additional here...)

I've also added a dependency on sphinx_rtd_theme in the environment.yml file as
the docs wouldn't build without it.
@fujiisoup
Copy link
Member

This looks clean :)

In the smaller window width, the table wrapping seems disabled.
Maybe we can also improve this?

(For the easier review, here are the screenshots)
selection_010

selection_011

@typesanitizer
Copy link
Author

@fujiisoup I can lower the threshold at which it changes from reflow → horizontal or remove it entirely.

  1. If I remove it, you will have a bad experience at < 500px width as the table clips on the right side and you can't even scroll to the right to see the whole thing. The picture below is at 440px width with reflow.
    440px

  2. If I lower the threshold, horizontal scrolling will activate only below 500px width, but the cost is extra vertical space between 767px (the current threshold) and 500px because of excessive wrapping. The picture below is at 500px width with reflow.
    500px

Should I with option 2? (The 500px number is somewhat arbitrary, I just guessed it based on a few trials. Of course, any appropriate number between 440 and 500 will do.)

@shoyer
Copy link
Member

shoyer commented Nov 20, 2017

Thanks for putting this together!

I just gave looked at a preview in Chrome. Here's the progression I see, as I shrink the width of the browser window:

  1. Nice wrapping on multiple lines:

  1. Increasingly shrunk right column, but no scrolling yet:

  1. At some critical width, table of contents disappears and table wrapping is again disabled:

My thought: we should definitely have a minimum width for the right column (maybe a few hundred px?). But it would be nice if we had consistent behavior for the API docs table, regardless of whether or not the table of contents fits.

@typesanitizer
Copy link
Author

@shoyer Currently that wrap → horizontal threshold is set to 767px in the CSS, which is why your third picture and fujisoup's second picture show the horizontal table.

@media screen and (min-width: 767px) {

Coincidentally (or maybe by design, idk), this 767px threshold is close to the TOC disappearing threshold. I propose lowering the threshold to 500px, or thereabouts. I haven't committed those changes yet; I'm waiting for the green light.
If we make that change, at extremely narrow widths (such as 440px - my first image) it will use a horizontal table (not shown, wrapping shown to illustrate that it looks bad). However, so long as it is readable with wrapping (which kinda' appears to be true at 500px width in the second image), it will wrap.
Sorry if I didn't make this clear; my second picture has no TOC but still has wrapping. It's not a relatively wide window with wrapping and the TOC cropped out.

@shoyer
Copy link
Member

shoyer commented Nov 20, 2017

@theindigamer OK, that makes more sense. I wonder, is there a way to set that cutoff threshold after subtracting the size of the TOC if shown? Or is that too complicated for CSS?

@typesanitizer
Copy link
Author

Do you mean something like

if toc.is_shown:
    threshold = foo - toc.width
else:
    threshold = bar
@media screen and (min-width: .threshold.width) {
/* same stuff as in commit */

or something else? I think arithmetic should be doable easily using the calc() function. For the conditional, I think we have to modify the HTML too (I don't know much about CSS so I'm simply following StackOverflow). However, we could just copy the theme's conditional statement as an approximate solution.

I've been looking at the CSS and the value of the sidebar's width is set using wy-nav-side.width which is 300px right now (line 4126 in sphinx_rtd_theme.css). If I understand correctly, the logic it is using the make the toc disappear is in lines 4251 - 4260.

4251 @media screen and (max-width: 768px) {
     [...]
      .wy-nav-side {
       left:-300px
4260  }
     [...]

I suppose this just moves the sidebar outside the display area (it has fixed width). My guess is that we can't refer to this 768px value directly because it isn't saved in any attribute, so we will need to copy the whole @media screen and (max-width: 768px) wherever we want to make that conditional check and hope it doesn't break in the future.

@keewis
Copy link
Collaborator

keewis commented Mar 26, 2021

We switched to a different theme in #4835 so this PR might not be necessary anymore. Should we close it? cc @andersy005

@andersy005
Copy link
Member

Yes, I think this PR can be closed

@keewis
Copy link
Collaborator

keewis commented Mar 26, 2021

let's close this, then. Thanks for the effort, @typesanitizer.

@keewis keewis closed this Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tables in docs make looking up functions unnecessarily hard
5 participants