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

Docutils 0.18 issue fixes #1351

Closed

Conversation

benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Oct 5, 2022

Fixes #1322 (forked from and blocked by #1304)

We take the same approach as @pradyunsg did with Furo and add additional styling for the new <aside> structure. The new DOM elements are harder to structure as a grid to match the former structure.

I'm quite happy with the new setup, as it allows easier mobile friendly setup and longer labels without causing issues with wrapping in a grid setup.

How footnotes look with docutils 0.18 and new SASS code:

image

@benjaoming benjaoming added the Design Design or UX/UI related label Oct 5, 2022
@benjaoming benjaoming added this to the 1.2 milestone Oct 5, 2022
@benjaoming benjaoming requested a review from a team as a code owner October 5, 2022 12:03
@agjohnson
Copy link
Collaborator

I'd put preference on retaining the same styling as docutils 0.17, there is a lot of vertical space in the list now. Also, this would be a 3rd style implementation to footnotes as html4/html5 also have differing styles to footnotes.

@benjaoming
Copy link
Contributor Author

I can try to match the styling from html5writer <dl> but will still be a 3rd implementation :)

Whenever I look at the SASS code and the setup I get a feeling that it's all gonna get refactored/rewritten anyways, so I automatically try to keep the effort minimal. I agree that the final result can improve visually (the current <dl> version also has a few glimpses btw).

@pradyunsg
Copy link

FWIW, if you find a way to avoid having the additional vertical whitespace, I'd like to borrow that back to Furo. I know those issues are closed, but I've not had the bandwidth to investigate the footnotes changes in detail -- whether they fixed my needs. It definitely doesn't help that docutils is maintained on SourceForge which... I find fugly. :)

I believe docutils hasn't figured out a way to stylise this in the manner we want either: https://docutils.sourceforge.io/docs/user/rst/demo.html#footnotes

@pradyunsg
Copy link

FWIW, two things have happened:

  1. Furo has better CSS for footnotes.
  2. I better understand CSS grids. :)

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to review. Setting this as blocked as I don't think we should go out with the fix until the display issues are resolved. This task is on my sprint.

@benjaoming
Copy link
Contributor Author

benjaoming commented Nov 14, 2022

This also made me have to look at CSS grids 🥲 However, I did not find a way to create a grid, since the variable DOM structure with label, definition or label, backrefs, definition was too hard a nut to crack for me. @pradyunsg I was looking at pradyunsg/furo#576 and guess you also went with float: left finally?

I tried to make the floats work like this...

before (docutils 0.17):

image

after (docutils 0.18):

image

I think that the result is acceptable. If there are several long paragraphs for a single footnote, the wrapping will not align. But I think that will belong to an absolute edge case 🤷

@benjaoming
Copy link
Contributor Author

benjaoming commented Nov 14, 2022

Notice also that the : is gone.. I didn't think it was worth bringing back, not least because it was already weirdly rendered in docutils 0.17. We could use the :before declaration on something like > p:first-of-type, however first-of-type only works in CSS3.

@benjaoming
Copy link
Contributor Author

benjaoming commented Nov 14, 2022

Edited: Circle CI issues have occurred because of a new release of sphinxcontrib-httpdomain that broke in Sphinx 1.7.

@agjohnson
Copy link
Collaborator

This also made me have to look at CSS grids

CSS grid is the best way to solve this, and avoids making a third implementation of the citation list -- html5 writer current uses CSS grid and html4 doesn't need floating/grid as it's a table.

Notice also that the : is gone

There is a selector that we can use (:has()), but it's not supported on Firefox, so I'd say that's a no go. It's fine to drop these for now.

We could use the :before declaration on something like > p:first-of-type

This wouldn't quite work, it would be grouped with the p instead of the span.backrefs. CSS3 features are fine as long as they are implemented. first-of-type has good enough support it's usable otherwise.

https://caniuse.com/?search=first-of-type


On #1322 I mentioned I'd jump in to solve this and wanted an approach that uses CSS grid, similar to our current approach for the HTML5 writer in docutils < 0.18. I have a PR I'll put up with the fix but it could probably use some more back testing.

@benjaoming
Copy link
Contributor Author

I'll close this PR and move the test fixes.

@agjohnson great work on the CSS grid!

Regarding :, I think that the footnotes lists look better without colons + the colon is unnecessary + the previous solution with : looked weird, especially with backrefs included.

@agjohnson
Copy link
Collaborator

Yeah the colon came from the html4 implementation, so was just added for parity. I agree that it looks unnecessary though, I'm also 👍 on dropping it

@benjaoming
Copy link
Contributor Author

Superseded by #1383 and #1381

@benjaoming benjaoming closed this Nov 16, 2022
@agjohnson
Copy link
Collaborator

FWIW, if you find a way to avoid having the additional vertical whitespace, I'd like to borrow that back to Furo.

@pradyunsg sounds like you poked around with CSS grids a bit, but if you are still looking for an example:

aside.footnote, aside.citation, div.citation
display: grid
// Two any width columns for label and backrefs, a noop column to expand
// to fill the width allocated to the first two columns, and then the
// content column for the paragraphs.
grid-template-columns: auto auto minmax(0.65rem, auto) minmax(80%, 95%)
& > span.label
grid-column-start: 1
grid-column-end: 2
& > span.backrefs
grid-column-start: 2
grid-column-end: 3
grid-row-start: 1
grid-row-end: 3
& > p
grid-column-start: 4
grid-column-end: 5

I would really have preferred docutils output more wrapping elements to make styling this easier. There's only so much you can do with the existing output structure, even with grids.

@benjaoming
Copy link
Contributor Author

@agjohnson in general, DOM changes in docutils without theme maintainer consultation is not nice. Once they have finished moving docutils to a more modern platform (work is ongoing to move docutils to git), perhaps it can be easier to be included in changes that affect the DOM.

@benjaoming benjaoming deleted the docutils-0.18-issue-fixes branch November 21, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Design or UX/UI related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docutils 0.18 support issues
4 participants