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

readthedocs - table formatting under 1.1 #2028

Closed
keiranmraine opened this issue Mar 20, 2020 · 9 comments · Fixed by #2356
Closed

readthedocs - table formatting under 1.1 #2028

keiranmraine opened this issue Mar 20, 2020 · 9 comments · Fixed by #2356
Labels
Bug Theme-readthedocs Issues specifically involving the readthedocs theme.

Comments

@keiranmraine
Copy link

The formatting under 1.1 has changed quite dramatically for the readthedocs option. Is this intentional?

Table from 1.0.4:

Screen Shot 2020-03-20 at 16 23 04

Table from 1.1:

Screen Shot 2020-03-20 at 16 23 15

My mkdocs.yml (variables expanded in bash script):

site_name: CI reports - $CI_PROJECT_PATH
repo_url: $CI_PROJECT_URL
repo_name: GitLab
edit_uri: null
site_description: CI reports for $CI_PROJECT_PATH
theme:
     name: readthedocs
nav:
$NAV
@waylan
Copy link
Member

waylan commented Mar 20, 2020

What is the behavior of the upstream Sphinx theme?

If the new behavior match upstream, then this is an intentional change. The MkDocs theme should not include anything which is not supported upstream. However, if this is a regression from the upstream Sphinx theme we may need to add some additional rules to theme_extra.css. A PR is welcome.

@waylan waylan added Needs confirmation Theme-readthedocs Issues specifically involving the readthedocs theme. labels Mar 20, 2020
@keiranmraine
Copy link
Author

The sphinx theme stable demo shows the tables to have the clean formatting (as desired):

https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/lists_tables.html#option-lists

@waylan
Copy link
Member

waylan commented Mar 24, 2020

Thanks for the link. Sure enough we have a problem with table formatting. The way this works is that we provide the upstream CSS in theme.css. That file comes directly from upstream without modification. We then have a few fixes specific to MkDocs in theme_extra.css. However, the few rules in theme_extra.css have not changed between versions 1.04 and 1.1. What has changed is that theme.css was updated to a more current version from upstream. Doing a little spelunking with the inspector, it appears that all style rules for tables in theme.css are applied to .rst-content table.docutils whereas previously they simply used .rst-content table. Of course, Markdown does not assign a docutils class to tables so the rules are being ignored.

Presumably, a fix for this would be to copy all relevant rules from theme.css to theme_extra.css and alter them to be applied to .rst-content table. Under no circumstances should we edit theme.css directly. A PR is always welcome.

@waylan
Copy link
Member

waylan commented Mar 24, 2020

As an aside, the inconsistency in the code spans depicted in the screenshots above was addressed in #2029. This would be a similar type of fix.

@mattbrictson
Copy link

Doing a little spelunking with the inspector, it appears that all style rules for tables in theme.css are applied to .rst-content table.docutils whereas previously they simply used .rst-content table. Of course, Markdown does not assign a docutils class to tables so the rules are being ignored.

I don't think this is an issue with the CSS changing. The upstream table CSS has required the docutils class for quite some time. The problem is that mkdocs used to use JS to apply the docutils class to tables (see #257) but that was clobbered when the JS file was entirely deleted in 873b970#diff-be6164876d7aedfd5a20eab74b55c16d.

Was the removal of theme.js intentional? If so, could we bring back just the part of it that ensures table styles work? I.e.

$('table').addClass('docutils');

@mattbrictson
Copy link

I fixed this for my own site by adding the following:

extra_javascript:
  - js/extra.js
// js/extra.js

document.addEventListener("DOMContentLoaded", function() {
  document.querySelectorAll("table").forEach(function(table) {
    table.classList.add("docutils");
  });
});

@waylan
Copy link
Member

waylan commented Mar 27, 2020

Was the removal of theme.js intentional?

Technically it wasn't removed. rather it was replaced with the upstream JavaScript. However, looking at it now, it appears that the previous version included both some of the upstream code and some hacks to make it work with MkDocs. Perhaps we need to bring parts of the old hacks back. If course, that should happen in a separate file. The upstream file should remain unmodified from upstream.

@rockandska
Copy link

Hi

Table decoration is not the only thing who has changed, text surrounded by single quote too

In 1.0.4
image

In 1.1.0
image

Regards

@wodny
Copy link
Contributor

wodny commented Apr 7, 2020

@rockandska As already mentioned, see #2029.

jorisroovers added a commit to jorisroovers/gitlint that referenced this issue Sep 11, 2020
- Upgraded to latest mkdocs
- Updated rule documentation with more configuration examples
- Replaced triple backticks with single backticks for inline fixed text
- Replaced bash with sh for shell blocks
- Removed trailing hashes in markdown titles
- Workaround for mkdocs table formatting issue, see
  mkdocs/mkdocs#2028)
- Documented docker slowness issue (closes #129)
MarkBaker pushed a commit to PHPOffice/PhpSpreadsheet that referenced this issue Feb 15, 2021
I made a documentation change and noticed that the result when browsed
locally did not quite match what is seen when browsing from the web.
After some research, I found mkdocs/mkdocs#2028
That described my situation well and suggested adding an extra
javascript script to the configuration. This worked exactly as desired
on my local machine. This accounts for the presence of extrajs.js
and mkdocs.yml in this request.

In addition to the display problem, "mkdocs build" generates the
documentation into a directory which is not ignored by git. I added
that directory to .gitignore as part of this request.

Finally, since I don't know how exactly the documentation makes it
to production, I made an insignificant change to one doc file as
a sanity check.
waylan added a commit to waylan/mkdocs that referenced this issue Apr 7, 2021
Rather than including this is the JS form the upstream theme, the JS
has been included in a separate file `theme_extra.js` so that is is not
lost in a future update copied form upstream. Fixes mkdocs#2028.
waylan added a commit that referenced this issue Apr 8, 2021
Rather than including this is the JS form the upstream theme, the JS
has been included in a separate file `theme_extra.js` so that is is not
lost in a future update copied form upstream. Fixes #2028.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Theme-readthedocs Issues specifically involving the readthedocs theme.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants