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

fix autosummary tables and add a note about tabs' toctree #42

Merged
merged 7 commits into from Mar 3, 2022

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Mar 3, 2022

add rule to fix #35

I also added a note to close #33 (it was just a reminder issue anyway).

@2bndy5 2bndy5 changed the title fix autosummary tables fix autosummary tables and add a note about tabs' toctree Mar 3, 2022
@jbms
Copy link
Owner

jbms commented Mar 3, 2022

It looks like your css rule may match all references to Python objects, not just ones in autosummary tables. That may not cause any problems, but it might be nicer to only target autosummary tables.

Unfortunately it appears that autosummary does not add any class that may be used to identify it, so we would have to monkey patch e.g. https://github.com/sphinx-doc/sphinx/blob/b60caca9408bb778ec0b277eed105dab8cfae0bb/sphinx/ext/autosummary/__init__.py#L152 to add such a class.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2022

I could add much more to the selector, but I think a monkey patch would be overkill since other themes don't take that approach.

I'll re-examine the generated autosummary HTML tomorrow and modify the rule. As of right now it only looks for the last 2 matching children (without specifying element tag names).

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2022

I added more to the selector. I could could keep going by adding .md-typeset and code.classes:not(.highlight)

@jbms
Copy link
Owner

jbms commented Mar 3, 2022

Ultimately I think the issue is that autosummary adds no special styling other than longtable which might be used elsewhere, so there is no way to distinguish it from any other table that happens to contain a reference to a Python object in one of its cells.

In general when working on the api documentation styling for this theme/for tensorstore, I noticed that sphinx output lacks appropriate semantic classes in a lot of cases and had to fix that by monkey patching. I think in large part this is because extensions like autosummary that produce docutils output do not normally coordinate with themes, so they see no immediate benefit from adding extra classes.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2022

In the case of autosummary, users could use a custom template to add CSS classes or skip the rubric tables all together,

@jbms
Copy link
Owner

jbms commented Mar 3, 2022

Although they could potentially add a class themselves somehow, this theme wouldn't know about it, so we couldn't specify the selector based on it.

I think the best solution would be to upstream a change in sphinx to add a class to the table, since other themes may also want to specially style the autosummary tables. But in the meantime we could fix it ourselves via monkey patching.

@jbms
Copy link
Owner

jbms commented Mar 3, 2022

Is your objection to the monkey patching just that you want to avoid added complexity in this theme?

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2022

yes, but I'm lazy like that. I can't really object to your valid points.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2022

Just pointing out that other theme's don't have a problem with autosummary tables because (as you pointed out) they tend to inherit from a basic CSS - unlike this theme. The real problem this PR aims to fix is overriding the CSS selector from mkdocs-material (which was written in a broad way).

I see this as a problem with mkdocs-material - not as a problem with autosummary. It would seem that longtable is mostly for latex writers, but a repo-wide search reveals that autosummary is the only other python source that uses longtable (not sure if that helps).

There's a warning in the tabularcolumns directive doc about why longtables are used in latex.

@jbms
Copy link
Owner

jbms commented Mar 3, 2022

Okay, fair enough.

@jbms jbms merged commit 1ef121a into jbms:main Mar 3, 2022
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2022

I still raised this issue upstream. I'm now subscribed to sphinx-doc/sphinx#10234

@2bndy5 2bndy5 deleted the fix-autosummary-tables branch March 3, 2022 19:19
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2022

The CI skipped upload to pypi (actual) because the commit wasn't tagged. Is that something I'm allowed to do? I'd probably go with v0.3.1

The latest is v0.3 on pypi; the test pypi index still is getting these updates.

@jbms
Copy link
Owner

jbms commented Mar 3, 2022

Yes feel free to create tagged commits when you think a release is appropriate.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 6, 2022

It looks like there is a CSS fix (class exclusive to autosummary tables) slated for Sphinx v4.5.0. I'm weary of pinning this theme to Sphinx v4.5.0 just to satisfy this CSS specificity problem because it seems like an edge case to me. I guess we could recommend users require Sphinx v4.5.0 (when it is released). However, we would need to alter the rule to somehow optionally look for the new CSS class autosummary.

Would it make more sense to monkey patch autosummary tables for older extension versions prior to Sphinx v4.5.0 and adjust the CSS rule accordingly?

mhostetter added a commit to mhostetter/galois that referenced this pull request Mar 16, 2022
mhostetter added a commit to mhostetter/galois that referenced this pull request Mar 16, 2022
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.

Autosummary tables have strange line wrapping Tabs require first item to be a page header
2 participants