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

Infra: remove two of the five instances of title at top of pages #2532

Merged
merged 3 commits into from
Apr 19, 2022

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Apr 16, 2022

We currently have the PEP title shown four times at the top of each page:

image

https://peps.python.org/8

This PR removes the contents title on the left:

image

@hugovk hugovk added the infra Core infrastructure for building and rendering PEPs label Apr 16, 2022
@hugovk hugovk requested a review from AA-Turner as a code owner April 16, 2022 09:57
@AA-Turner
Copy link
Member

The idea behind the contents title on the left was as it is sticky -- so the title is always visible. Perhaps that is a bad rationale, but that was why I added it!

A

@hugovk
Copy link
Member Author

hugovk commented Apr 16, 2022

Ah right, I see. Yes, I think it could still be removed.

And I didn't mention the fifth place: the tab title :)

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Agreed; I'd been just thinking the same. Being stickied is the one benefit, yes, but its still :stickied" in the tab title and not that important to have visible all the time that we need so many copies floating around, and it doesn't help on narrow widths/mobile.

As a corollary, I might suggest eliding the full title from the breadcrumbs and just leaving the PEP number, which is much shorter while still getting the same point across, and avoiding one or possibly two extra lines on narrow widths/mobile.

Unless we could just sticky the breadcrumbs bar, which would be reasonably handy for information and navigation purposes and justify keeping it there, but that's not entirely trivial.

@hugovk
Copy link
Member Author

hugovk commented Apr 16, 2022

As a corollary, I might suggest eliding the full title from the breadcrumbs and just leaving the PEP number, which is much shorter while still getting the same point across, and avoiding one or possibly two extra lines on narrow widths/mobile.

Had thought the same :) I've pushed a commit to remove this too.

image

@CAM-Gerlach
Copy link
Member

As I've generally seen it recommended to include the current page in the breadcrumbs, and its particularly odd that it looks like its implied that the current page is the "PEP Index", what I'm suggesting is eliding the title but still including the PEP number, i.e. something like {{ title.split("-")[0].strip() }} in the template, so its Python > PEP Index > PEP NNN. That still fits on one line in the narrowest mobile browsers and doesn't repeat the title, but still makes clear what the current page is.

@hugovk
Copy link
Member Author

hugovk commented Apr 17, 2022

Got it, updated!

@hugovk hugovk changed the title Infra: remove one of the four instances of title at top of pages Infra: remove two of the five instances of title at top of pages Apr 17, 2022
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM again; thanks @hugovk !

@CAM-Gerlach CAM-Gerlach merged commit 53d6d59 into python:main Apr 19, 2022
@hugovk hugovk deleted the rm-contents-title branch April 19, 2022 07:34
onerandomusername added a commit to onerandomusername/monty-python that referenced this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Core infrastructure for building and rendering PEPs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants