Make the table of contents collapsed by default #2364

AA-Turner · 2022-02-26T00:19:16Z

Still in two minds about this one, but as a proof of concept.

If on mobile, the sidebar ToC goes to the bottom, so there's no immediate navigation mechanism.

A

CAM-Gerlach · 2022-02-26T00:31:40Z

Personally, if given the option between having a duplicate ToC and not, I'd ✔️ this PR, but what about making it collapsed by default, as suggested and discussed on the Discourse thread? It seems like we all agreed that approach would be the best of both worlds.

JelleZijlstra · 2022-02-26T03:21:37Z

Agree that a collapsed-by-default ToC would be most useful.

gvanrossum

Isn’t it confusing to use PEP 676 in the PR title?

AA-Turner · 2022-02-27T01:08:39Z

Isn’t it confusing to use PEP 676 in the PR title?

Hmm, perhaps. I don't want to break the pattern of 'PEP N: Description', and so I co-opted it for the PEP 676 implementation PRs. Happy to take suggestions for another prefix scheme.

(I'm against 'Infra' as it isn't a proper word, and 'Infrastructure' is far too long).

A

gvanrossum · 2022-02-27T01:22:39Z

Isn’t it confusing to use PEP 676 in the PR title?

Hmm, perhaps. I don't want to break the pattern of 'PEP N: Description', and so I co-opted it for the PEP 676 implementation PRs. Happy to take suggestions for another prefix scheme.

That pattern is supposed to indicate the PEP being modified, not the PEP that directs the changes. The pattern is pretty informal, so you could just start with PEP 249, PEP 512: ...

(I'm against 'Infra' as it isn't a proper word, and 'Infrastructure' is far too long).

That argument seem weak. :-)

CAM-Gerlach · 2022-02-27T02:07:26Z

That pattern is supposed to indicate the PEP being modified, not the PEP that directs the changes.

This is also how I interpreted it, and personally I've found it rather confusing, for the same reasons @gvanrossum did.

That argument seem weak. :-)

Agreed. Abbreviations are common, encouraged and sometimes (e.g. conventional commits) even required in PR titles and commit messages. So long as those likely to see and care about it know what it means (which AFAIK "infra" is a widely-used abbreviation in dev circles), I don't think it's terribly relevant if it's not "proper" Queen's English. After all, neither is "docs" or other abbreviations we regularly use.

hugovk · 2022-03-13T08:34:29Z

@AA-Turner Please could you rebase this so we get a build preview?

(Another huge benefit of build previews: easy to test on mobile!)

This reverts commit 8af9bf1.

AA-Turner · 2022-03-13T15:31:54Z

The ToC is now collapsed by default with a toggle:

https://pep-previews--2364.org.readthedocs.build/pep-0008/

A

gvanrossum · 2022-03-13T15:42:46Z

a) What does “PRS” mean?

b) I object to the styling of the table at the top with alternating gray and white background color. My mind always wonders why the gray ones get emphasis.

AA-Turner · 2022-03-13T17:02:14Z

a) What does “PRS” mean?

"PEP Rendering System", I've removed the prefix for simplicity.

b) I object to the styling of the table at the top with alternating gray and white background color. My mind always wonders why the gray ones get emphasis.

I copied this from the old python.org styling -- would it be better just to have them all white & remove the gray highlights?

A

gvanrossum · 2022-03-13T17:11:22Z

Okay, and Yes. Thank you!

hugovk · 2022-03-13T17:21:08Z

Here's a comparison of the old and new sites:

Before there were two greys: #f0f0f0 and #f6f6f6.

Now it's #eeeeee and #ffffff white, showing a larger difference.

AA-Turner · 2022-03-13T17:25:44Z

https://pep-previews--2425.org.readthedocs.build/ is a preview without the highlights, I might add an inner horizontal border though.

A

CAM-Gerlach · 2022-03-13T18:05:35Z

Here's a comparison of the old and new sites

At least in the PEPs table is a lot better for accessibility. Even to my eyes, I have a really hard time clearly separating them otherwise.

But I can see @gvanrossum concerns in the PEP headers. If we're going to remove it there, though, we should ad horizontal rules separating the fields, to make it easier to track what goes with what and make it look a little less odd and unstructured.

AA-Turner · 2022-03-13T18:23:18Z

we should add horizontal rules separating the fields

See the PR, I've done this.

A

CAM-Gerlach · 2022-03-13T19:11:28Z

See the PR, I've done this.

After having reviewed that one (and left a detailed review comment on it), I think you might have mistakenly pushed your changes to #2425 instead of this PR. If you want to move that over here, I can move my comment as well.

AA-Turner · 2022-03-13T19:55:20Z

Sorry for the confusion -- my fault for putting the discussion of field styling in this PR. I was referring to adding horizontal rules on each field, which I did -- you then commented that the colour should have greater contrast on said other PR.

This PR only actually deals with the ToC! If the implementation of the collapsed-by-default ToC here is OK, I will merge this PR to help clear up the discussion.

A

hugovk · 2022-03-13T20:10:48Z

If on mobile, the sidebar ToC goes to the bottom, so there's no immediate navigation mechanism.

On mobile, I see the collapsed Contents toggle near the top but not other ToC at the bottom. But that's fine, I wouldn't really expect it down there.

CAM-Gerlach · 2022-03-13T20:10:50Z

Sorry, I guess I was the one that got confused—I didn't see #2425 explicitly referenced anywhere on this PR (or on that one) and the discussion that prompted the change was here (on a PR rather than an issue), so I guess I was expecting to see those changes here. I'll review this one for just the collapsing ToC.

CAM-Gerlach

As can be seen in the preview, the Contents dropdown is not styled as intended, and thus looks rather odd.

Specifically, a h2 element wasn't used inside Summary for the title so that rule doesn't have any effect, and in any case the current content is redundant as h2 already has font-weight: bold;, while it needs display: inline to render on the same line as the drop-down arrow.

However, this still doesn't quite fix the issue, as the triangle drop-down marker is not sized or aligned with the rest of the text, but this too can be fixed by making summary the h2 size. Final result:

Alternatively, we could drop the h2 and just style summary directly, though that would be less DRY.

CAM-Gerlach · 2022-03-13T20:47:59Z

pep_sphinx_extensions/pep_theme/static/style.css

+details > summary > h2 {
+    font-weight: bold;
+}


Suggested change

details > summary > h2 {

font-weight: bold;

}

details > summary {

font-size: 1.6rem;

}

details > summary > h2 {

display: inline;

}

CAM-Gerlach · 2022-03-13T20:48:29Z

pep_sphinx_extensions/pep_processor/html/pep_html_translator.py

@@ -89,6 +89,17 @@ def depart_label(self, node) -> None:
        # Close the def tags
        self.body.append("</dt>\n<dd>")

+    def visit_bullet_list(self, node):
+        if isinstance(node.parent, nodes.section) and "contents" in node.parent["names"]:
+            self.body.append("<details><summary>Contents</summary>")


Suggested change

self.body.append("<details><summary>Contents</summary>")

self.body.append("<details><summary><h2>Contents</h2></summary>")

AA-Turner · 2022-03-14T01:50:49Z

As can be seen in the preview, the Contents dropdown is not styled as intended

Fixed now. I had the <h2> selector in the rule from testing -- the intent was to de-emphasise the contents, as it is now a navigation aid not a section proper, and a full H2 header looked odd with the small triangle.

A

CAM-Gerlach · 2022-03-14T03:04:50Z

It looks a little odd and out of place to me, thought I'm not 100% sold on the look of my suggestions either (i.e. the same as it looked previously). What do you all think @hugovk @JelleZijlstra @gvanrossum ? If you prefer the appearance above, I can implement my suggestions instead in another PR, or if you have other ideas, I'd be happy to help implement them in @AA-Turner 's absence.

a full H2 header looked odd with the small triangle.

To note, I address this above; it can be fixed by applying the size to the summary section as well.

hugovk · 2022-03-14T04:16:17Z

I think it's fine as is, and the screenshot above also looks okay. No strong opinion either way.

encukou · 2022-03-14T08:46:24Z

There's more than enough whitespace to set the lines apart visually. This isn't a dense multi-column table where eyes would need help to follow the lines.
How about deleting all the rules?

gvanrossum · 2022-03-14T15:50:45Z

+1 to Petr's suggestion.

FWIW the one usability improvement you could make in PEP 0 with the biggest effect would be to make PEP titles clickable. Having to aim at the little number in front of e.g. PEP 1 is awkward for someone with as clumsy a mouse as myself.

CAM-Gerlach · 2022-03-14T19:15:45Z

FWIW the one usability improvement you could make in PEP 0 with the biggest effect would be to make PEP titles clickable. Having to aim at the little number in front of e.g. PEP 1 is awkward for someone with as clumsy a mouse as myself.

@gvanrossum Agreed; implemented in #2429

the-knights-who-say-ni added the CLA signed label Feb 26, 2022

AA-Turner requested a review from a team February 26, 2022 00:19

gvanrossum reviewed Feb 26, 2022

View reviewed changes

CAM-Gerlach mentioned this pull request Mar 4, 2022

Meta: Clearly document current guidance for changing existing PEPs #2378

Merged

5 tasks

AA-Turner added 4 commits March 13, 2022 14:14

Fix links in section headings

a6a5187

Remove automatic table of contents

8af9bf1

Revert "Remove automatic table of contents"

92f3dc5

This reverts commit 8af9bf1.

Toggleable ToC

67355b2

AA-Turner force-pushed the drop-auto-toc branch from a765e87 to 67355b2 Compare March 13, 2022 15:25

AA-Turner changed the title ~~PEP 676: Remove automatic table of contents~~ PRS: Make the table of contents collapsed by default Mar 13, 2022

AA-Turner marked this pull request as ready for review March 13, 2022 15:30

AA-Turner requested review from brettcannon and malemburg as code owners March 13, 2022 15:30

AA-Turner removed request for brettcannon and malemburg March 13, 2022 15:31

AA-Turner changed the title ~~PRS: Make the table of contents collapsed by default~~ Make the table of contents collapsed by default Mar 13, 2022

CAM-Gerlach requested changes Mar 13, 2022

View reviewed changes

Fix rule

5f5f2a7

AA-Turner merged commit 13aa4b1 into python:main Mar 14, 2022

CAM-Gerlach mentioned this pull request Mar 14, 2022

PEP 0: Make PEP titles in table clickable links #2429

Merged

CAM-Gerlach mentioned this pull request Apr 19, 2022

Change "Contents" to "Table of Contents" ? (Or delete it?) #2540

Closed

erlend-aasland mentioned this pull request May 13, 2022

pep8/greppable exception messages erlend-aasland/peps#1

Closed

erlend-aasland mentioned this pull request Jun 27, 2022

pep 687/mark as accepted erlend-aasland/peps#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the table of contents collapsed by default #2364

Make the table of contents collapsed by default #2364

AA-Turner commented Feb 26, 2022

CAM-Gerlach commented Feb 26, 2022

JelleZijlstra commented Feb 26, 2022

gvanrossum left a comment

AA-Turner commented Feb 27, 2022 •

edited

Loading

gvanrossum commented Feb 27, 2022

CAM-Gerlach commented Feb 27, 2022

hugovk commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

gvanrossum commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

gvanrossum commented Mar 13, 2022

hugovk commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

CAM-Gerlach commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

CAM-Gerlach commented Mar 13, 2022 •

edited

Loading

AA-Turner commented Mar 13, 2022

hugovk commented Mar 13, 2022

CAM-Gerlach commented Mar 13, 2022 •

edited

Loading

CAM-Gerlach left a comment •

edited

Loading

CAM-Gerlach Mar 13, 2022

CAM-Gerlach Mar 13, 2022

AA-Turner commented Mar 14, 2022

CAM-Gerlach commented Mar 14, 2022

hugovk commented Mar 14, 2022

encukou commented Mar 14, 2022

gvanrossum commented Mar 14, 2022

CAM-Gerlach commented Mar 14, 2022

	self.body.append("<details><summary>Contents</summary>")
	self.body.append("<details><summary><h2>Contents</h2></summary>")

Make the table of contents collapsed by default #2364

Make the table of contents collapsed by default #2364

Conversation

AA-Turner commented Feb 26, 2022

CAM-Gerlach commented Feb 26, 2022

JelleZijlstra commented Feb 26, 2022

gvanrossum left a comment

Choose a reason for hiding this comment

AA-Turner commented Feb 27, 2022 • edited Loading

gvanrossum commented Feb 27, 2022

CAM-Gerlach commented Feb 27, 2022

hugovk commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

gvanrossum commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

gvanrossum commented Mar 13, 2022

hugovk commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

CAM-Gerlach commented Mar 13, 2022

AA-Turner commented Mar 13, 2022

CAM-Gerlach commented Mar 13, 2022 • edited Loading

AA-Turner commented Mar 13, 2022

hugovk commented Mar 13, 2022

CAM-Gerlach commented Mar 13, 2022 • edited Loading

CAM-Gerlach left a comment • edited Loading

Choose a reason for hiding this comment

CAM-Gerlach Mar 13, 2022

Choose a reason for hiding this comment

CAM-Gerlach Mar 13, 2022

Choose a reason for hiding this comment

AA-Turner commented Mar 14, 2022

CAM-Gerlach commented Mar 14, 2022

hugovk commented Mar 14, 2022

encukou commented Mar 14, 2022

gvanrossum commented Mar 14, 2022

CAM-Gerlach commented Mar 14, 2022

AA-Turner commented Feb 27, 2022 •

edited

Loading

CAM-Gerlach commented Mar 13, 2022 •

edited

Loading

CAM-Gerlach commented Mar 13, 2022 •

edited

Loading

CAM-Gerlach left a comment •

edited

Loading