Skip to content
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

Wrap table cells for a better responsive table experience + fix print #432

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Jun 15, 2017

Fixes

Adds:

  • Wrapping for <td>s on desktop sizes
  • Visible overflow and wrapping on printed media

Does not wrap <th> on purpose, this leaves a bit of control for people who wanna force the width of certain columns.

Please observe the great amount of projects working around the issue with related commits, issues and PRs.

Quotes:

@kmike

Why not have relevant CSS fixed by default? Are there cases when it is desirable to truncate the information in table, be it a tablet screen or a computer screen? On tablets existing tables are even worse because horizontal scroll of individual elements is hard on touch interfaces. On macs there is no indication that table can be horizontally scrolled because scrollbars are hidden by default; they don't appear even on hover, you need to actually start scrolling to see the scrollbars.

@agjohnson

This was a design decision for the theme around table wrapping, though I agree that the experience around wide tables on large displays is generally poor.

Before

I agree that this may look nicer, but that's only because the example table doesn't have a lot of text in its <td>s!

screenshot from 2017-06-15 22-54-02

After

screenshot from 2017-06-15 22-53-42

Printing

There's now a higher probability that tables will print:

screenshot from 2017-06-15 23-44-06

FAQ

Q: Why no overflow: visible on desktop tables?

A: Because horizontal scrolling is completely off in the theme, so the table would be cropped like this :/

screenshot from 2017-06-15 23-14-24

@benjaoming
Copy link
Contributor Author

I'm also convinced that for readability, now that wrapping is introduced, we need vertical-align: top for <td> which has to go through a separate rule.

Displays like this:

screenshot from 2017-06-15 23-40-19

@dpdani
Copy link

dpdani commented Jun 16, 2017

Hi,
I probably have a corner case with this issue: in my docs, there's a table in which the first column always has preformatted text which is pretty long and overlaps with the next table cell.
See here at page 12 and forward.
Would this corner-case be fixed by this PR?

@benjaoming
Copy link
Contributor Author

@dpdani No, the CSS that changes print media is about printing HTML through a browser, your report is generated with a different ruleset for Latex. I'm not sure why, but if you read up on how Latex works and about how in penalises and optimizes its layout to have as little penalty as possible, you'll get the idea why this can happen.

@dpdani
Copy link

dpdani commented Jun 16, 2017

@benjaoming Ok thank you.

@Blendify
Copy link
Member

FYI, this is a sensitive change, so the review might take a while as we test.

@benjaoming
Copy link
Contributor Author

@Blendify sounds good! Thanks for taking time to test it!

@benjaoming
Copy link
Contributor Author

@ericholscher @agjohnson questions or concerns here that I can respond to?

@Blendify Blendify added this to the v0.2.6 milestone Jul 30, 2017
@mitya57
Copy link
Contributor

mitya57 commented Aug 20, 2017

+1 for this change. I was wondering why one of the tables looks so weird, and found that white-space: nowrap rule. Changing it to normal makes it look much better.

Before:
before

After:
after

benjaoming added a commit to benjaoming/kolibri that referenced this pull request Aug 21, 2017
…d theme_overrides.css without overriding the entire theme when on remote RTD server

See:
readthedocs/readthedocs.org#2116
readthedocs/sphinx_rtd_theme#432
@krzychb
Copy link

krzychb commented Apr 8, 2018

Hi RTD team,

What are the prospects to have this PR merged?

This bug is really annoying.

Here is my story:

Looking forward to see this PR merged.
Documentation by RTD will be better without crappy tables like below you have to scroll left and right to be able to read it:

image

@Blendify
Copy link
Member

Blendify commented Apr 9, 2018

We will have a look as soon as the next release is out.

1 similar comment
@Blendify
Copy link
Member

Blendify commented Apr 9, 2018

We will have a look as soon as the next release is out.

igrr pushed a commit to espressif/esp-idf that referenced this pull request Apr 17, 2018
…e table cells. Each row is extended to fit the text as a single line. For a table with multiple columns and long sentences the table may get wide, with long scrollbar underneath and is not convenient to read. This MR makes the text wrap inside table cells. There is an open PR to fix this issue - readthedocs/sphinx_rtd_theme#432.
@agjohnson agjohnson modified the milestones: v0.4.0, 0.5 Jun 7, 2018
jkglasbrenner pushed a commit to jkglasbrenner/datamaterials-neighbormodels that referenced this pull request Aug 7, 2018
  * Adopt RTD theme for docs HTML rendering.
  * Refactored the settings related to generating the table of contents as this
    is rendered somewhat differently in the RTD theme.
  * Update RTD configuration settings, just install requirements in setup.py
    file instead of using environment.yaml.
  * Generate individual rst files for all individual functions.
  * Add CSS file to fix wrapping issues as discussed here,
    readthedocs/sphinx_rtd_theme#432 (comment).
  * Revise content on index.rst file.
@benjaoming benjaoming force-pushed the responsive-tables branch 2 times, most recently from dfd6baf to a546ace Compare October 1, 2018 10:33
docs/make.bat Outdated Show resolved Hide resolved
@benjaoming
Copy link
Contributor Author

@agjohnson

agjohnson removed the PR: ready for review label 14 days ago

What does that mean?

PR is rebased to latest master.

@Blendify
Copy link
Member

Blendify commented Oct 1, 2018

@agjohnson

agjohnson removed the PR: ready for review label 14 days ago

What does that mean?

PR is rebased to latest master.

This can be ignored, it was automated. All the labels for rtfd were synced so some were removed.

@stsewd
Copy link
Member

stsewd commented Oct 1, 2018

@benjaoming we don't use that label anymore on rtd, now we use work in progress (not ready for review) or 'not label' (ready for review if ci pass) flow

@benjaoming
Copy link
Contributor Author

Okay, it's ready for review... and has been since June 2017 😄

@lalten
Copy link

lalten commented Oct 1, 2020

What are the chances of getting this reviewed/merged in the foreseeable future?

@benjaoming
Copy link
Contributor Author

Adding this for additional reference to the extensive workarounds for this issue: OpenShot/openshot-qt#1272

@benjaoming benjaoming requested a review from a team as a code owner August 27, 2022 07:48
benjaoming added a commit to benjaoming/sphinx_rtd_theme that referenced this pull request Aug 27, 2022
benjaoming added a commit to benjaoming/sphinx_rtd_theme that referenced this pull request Aug 27, 2022
benjaoming added a commit to benjaoming/sphinx_rtd_theme that referenced this pull request Aug 27, 2022
@benjaoming
Copy link
Contributor Author

benjaoming commented Aug 27, 2022

Fixed a bit of SASS syntax and merged latest master in.

Understanding that this PR will change how existing tables are displayed by default, but I think it's an entirely desirable change. One hack will still exist to keep very wide columns because nowrap is maintained on <th> (I would actually want to remove that if no one objects)

image

What do other themes do?

Short answer: They wrap text in table cells.

As there aren't any specific worries associated with the change and I see other themes with text wrap in table cells, I think this would still be the way forward.

Examples of other themes with wrapping:

(I didn't find an example of the opposite)

@agjohnson
Copy link
Collaborator

Bumping the release on this, 1.1 is a bug fix release.

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.

8 participants