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

PEP 676: 'dark mode', documentation, spec update, implementation update #2239

Merged
merged 30 commits into from Jan 16, 2022

Conversation

AA-Turner
Copy link
Member

A lot of little things -- I could open individual PRs if preferred.

This PR adds

  • 'dark mode' as requested by @warsaw and others (preview at https://aa-turner.github.io/peps/pep-0008/). Note in keeping with the theme of simplicity I have stuck with user-agent based detection for the moment -- if your device requests a dark colour scheme, it should get one. A toggle button could be added, but I'd rather see if there is demand for it than complicating things quite a bit by adding one.
  • a specification update to note that PR previews SHOULD be provided
  • a few small implementation updates

It also proposes an initial stab at documentation -- more for review than for merging here, as the final location isn't yet decided (it might go into the dev guide). It is a very particular stylistic approach, with a descriptive slant -- partly to try to stop me from going into far too much detail when writing. Would welcome feedback, on the scale of "no changes at all" to "start over entirely".

A

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.

Excellent overall! Mostly just minor typos, punctuation, grammar, and formatting, plus some small clarifications and a bit of light re-phrasing, along with a handful of more substantive questions, comments and suggesitons.

As a general comment, I'm not sure its advisable to include the prompt in the code blocks; I don't have a source handy at the moment but IIRC at least some style guides and rst linters explicitly discourage including it as it simply adds noise and will vary between OSes, shells, users, etc. But its not the end of the world.

docs/build.rst Outdated Show resolved Hide resolved
docs/build.rst Outdated Show resolved Hide resolved
docs/build.rst Outdated Show resolved Hide resolved
docs/build.rst Show resolved Hide resolved
docs/build.rst Outdated Show resolved Hide resolved
docs/rendering_system.rst Outdated Show resolved Hide resolved
docs/rendering_system.rst Outdated Show resolved Hide resolved
docs/rendering_system.rst Outdated Show resolved Hide resolved
docs/rendering_system.rst Show resolved Hide resolved
docs/rendering_system.rst Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member Author

I'm not sure its advisable to include the prompt in the code blocks

I felt a bit funny about this, but the Pygments docs seemed to indicate it was required. Not fussed to drop it or keep it.

A

@merwok
Copy link
Member

merwok commented Jan 14, 2022

About the prompts, note that Python docs have a plugin to hide prompts and outputs for easier copying.
So if you were influenced by the Python docs, their style may not apply everywhere.

@warsaw
Copy link
Member

warsaw commented Jan 14, 2022

I'm pressed for time, but first of all, thank you for dark mode support! Second, let me respond to this:

'dark mode' as requested by @warsaw and others (preview at https://aa-turner.github.io/peps/pep-0008/). Note in keeping with the theme of simplicity I have stuck with user-agent based detection for the moment -- if your device requests a dark colour scheme, it should get one. A toggle button could be added, but I'd rather see if there is demand for it than complicating things quite a bit by adding one.

Even though I prefer dark mode, I do think a toggle button is useful. There are cases where I might need to view the light mode, e.g. if I'm developing the documentation and want to see how it looks in both cases, or if I'm presenting the documentation in a video chat and dark mode doesn't show up well there. Sure there are other ways to switch between dark and light modes, but a toggle makes the most sense and is the most discoverable.

I use furo and it has a toggle, although its 3-way toggle always seems confusing to me.

@AA-Turner
Copy link
Member Author

I also added an explicit CC-0 / public domain declaration to the pep_sphinx_extensions code & associated miscellany, as I realised there is no repository-wide licence (likely due to that every PEP contains a CC-0/PD declaration within it).

A

@merwok
Copy link
Member

merwok commented Jan 16, 2022

#!/usr/bin/env python3 should be compatible with most operating systems and I think also the py launcher.

The informational PEP is not fully clear: https://www.python.org/dev/peps/pep-0394/#for-python-script-publishers

@AA-Turner
Copy link
Member Author

Thanks Éric!

@CAM-Gerlach
Copy link
Member

Sorry for all the churn, and thanks for bearing with me!

I'm using pygments' monokai theme for dark mode syntax highlighting -- everything comes as a package. Monokai seems popular though, I've seen it fairly often used elsewhere.

Yep, its one of the most popular out there haha; IIRC I used to use it myself long ago.

Not highlighting builtin functions differently is ultimately a subjective choice (or a minor limitation of the theme), but the comment color is a much more serious problem. Certainly, some users may prefer less contrast on comments for their personal choice of syntax highlighting theme, but in the hardcoded color scheme of a widely used website, it raises serious accessibility and readability concerns (and as we know, "readability counts").

You can override it in your style.css with (since pygments_dark.css is leaded after style.css):

.highlight .c1 {
	color: #d0c0a0 !important;
}

I'll add the shebang line.

Thanks! FYI, the files need to be marked executable so the shebang will actually work. For that, run git update-index --chmod=+x build.py and commit. While you're at it, I suggest adding the shebang and execute bit to generate_rss.py as well for consistency, since pep2rss.py has it as well—sorry for not explicitly mentioning that before.

Should it be #!/usr/bin/env python or #!/usr/bin/env python3? I believe the second, although I don't know what e.g. debain, CentOS, etc are doing re updating the python command to point to Python 3.

Sorry, I'd left the 3 off due to a copy/paste error—my mistake!

I also added an explicit CC-0 / public domain declaration to the pep_sphinx_extensions code & associated miscellany, as I realised there is no repository-wide licence (likely due to that every PEP contains a CC-0/PD declaration within it).

The one issue with CC0 that hinders its use with software is that it explicitly doesn't grant patent rights, so theoretically you (or someone else) could patent it or certain aspects of it, creating a legal hazard. But obviously, I assume you don't plan to do that, and realistically, this is more so an issue that could happen with any PEP rather than this Sphinx extension.

For the avoidance of doubt, as the other person I could find who made (minor) contributions to the docs and code in question, I hereby irrevocably release any copywritable portions of my work to the public domain or under the Creative Commons Zero v1.0 Universal (CC0-1.0) license, whichever is more permissive.

@hugovk
Copy link
Member

hugovk commented Jan 16, 2022

Looking good! I agree the code comments are hard to read. This accessibility checker at https://webaim.org/resources/contrastchecker/ says:

WCAG 2.0 level AA requires a contrast ratio of at least 4.5:1 for normal text and 3:1 for large text. WCAG 2.1 requires a contrast ratio of at least 3:1 for graphics and user interface components (such as form input borders). WCAG Level AAA requires a contrast ratio of at least 7:1 for normal text and 4.5:1 for large text.

Current #75715e on #1D1E19

Proposed #d0c0a0 on #1D1E19

Also

Chrome devtools also has a contrast checker. Right click and inspect element, click on the colour square and scroll down a bit:

Oh, and you can click the little icon to the right of 🚫 to get a suggested colour:

There's a few others whose contrast could be tweaked too, the ones used for == and in here:

Using Chrome's suggestions:

Compare with GitHub:

# Wrong:
if foo == 'blah': do_blah_thing()
for x in lst: total += x
while t < 10: t = delay()

And https://wave.webaim.org/report#/https://aa-turner.github.io/peps/pep-0008/ is a really good tool for checking a webpage's accessibility.


I realise this is almost the definition of bikeshedding :) But accessibility is important, I think at least the comments need changing.

Another idea is to use a different Pygments style. For example, Furo uses Native style: https://www.attrs.org/en/stable/examples.html#defaults

@AA-Turner
Copy link
Member Author

Thanks for the detailed feedback Hugo! I think I'd prefer to use a different theme than start to get into overriding specific class colours -- I assume Pygments will be reasonably stable in their naming, but I don't know what guarantees there are there. I don't have time to find citations, but I seem to remember !important has been advised against.

Are you able to check if the Native style would pass the various tests you outlined? I can do so, but it would be much later today or into tomorrow.

A

@hugovk
Copy link
Member

hugovk commented Jan 16, 2022

How's this? At least I find the code comments easier to read with Native. The WAVE tool still reports a couple of low contrast things but much fewer than for Monokai and they're not too far off.

Monokai

Native

@CAM-Gerlach
Copy link
Member

Thanks @hugovk ! I'd checked it with the WebAIM checker and Chrome's site linter, but I didn't know about the full-site check or the color suggestion.

Native style looks great; wouldn't be my preference for code in an editor but it fits in way better than Monokai with the rest of the PEP site, which kinda stuck out a bit. I don't see any further major problems worth bikeshedding over in terms of that; the only remaining issue I noticed is:

FYI, the files need to be marked executable so the shebang will actually work. For that, run git update-index --chmod=+x build.py and commit. While you're at it, I suggest adding the shebang and execute bit to generate_rss.py as well for consistency, since pep2rss.py has it as well—sorry for not explicitly mentioning that before.

Thanks again!

@AA-Turner
Copy link
Member Author

AA-Turner commented Jan 16, 2022

Native also visually looks far better with the rest of the site in my opinion, let's make the change. (You can tell I put a lot of thought into choosing the Pygments style xD).

executable

I did catch this, but I was travelling to Scotland today so away from my computer for a while to make the change -- I'll do this now. We don't have these executable bit troubles on windows :P (I will accept no other criticisms!)

A

@CAM-Gerlach
Copy link
Member

I did catch this, but I was travelling to Scotland today so away from my computer for a while to make the change -- I'll do this now. We don't have these executable bit troubles on windows :P

Heheh, well that's cause we (speaking as a developer who still mainly uses Windows on my old personal laptop) can't do it at all, heh. I have to keep that git command handy (and constantly have to refer to it myself, since I can never remember the exact form...should make it an alias) to fix such cases.

(I will accept no other criticisms!)

Well achtully I do have one more thing...

Just kidding! Looks good to go from my side now. Thanks so much for bearing with me!

@Mariatta @warsaw @JelleZijlstra Care to review?

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 now @AA-Turner , at least in terms of the docs, CLI, theme and other ancillaries that I reviewed!

@AA-Turner
Copy link
Member Author

so theoretically you (or someone else) could patent it or certain aspects of it,

I don't know if there's a way to resolve this, and as you point out every PEP uses the CC0 licence. Equally, I don't intend to patent this work, and prior art certainly exists for large parts, the "innovation" was gluing things together in the right order to make Sphinx happy.

Hugo especially, and Éric and Pablo contributed ideas (maybe code, but I can't remember) during the initial implementation stages. I'm not sure if everyone is required to declare their contributions to be PD, though -- I have a limited at best grasp of UK copyright law, let alone American!

A

const pygmentsNormal = document.getElementById("pyg");
const pygmentsDark = document.getElementById("pyg-dark");

const makeLight = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an explicit decision on what browsers are supported for PEPs? https://caniuse.com/arrow-functions

Copy link
Member

Choose a reason for hiding this comment

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

(Though to be clear the browsers that don't support arrow functions are really old, 2016 or earlier, and the site would probably work fine even if this file crashed, so it's not too important.)

Copy link
Member Author

Choose a reason for hiding this comment

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

JS is only required for the toggle button and restoring state -- UA detection for dark mode will work without it.

Annecdotally Sphinx is also looking to modernise its JS stack / drop old browsers (sphinx-doc/sphinx#7405 (comment)), so even if we wanted to support is being droped upstream.

A

Copy link
Member Author

Choose a reason for hiding this comment

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

I admit I haven't tested in ancient browsers, but I expect a syntax error in JS wouldn't cancel the loading of the DOM / HTML itself.

I tried really hard to have no JS at all (partially to avoid these questions!), but in the end I couldn't find a way to do the toggle button without it.

A

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, CSS Variables which are extensively relied upon by the stylesheet too, and for which it lacks fallbacks, are supported by about the same browsers as arrow functions, so it seems reasonable to allow them.

I admit I haven't tested in ancient browsers, but I expect a syntax error in JS wouldn't cancel the loading of the DOM / HTML itself.

Yup, browsers are designed to be very tolerant of errors. Your browser likely runs into JS errors not infrequently, and the only indication you'll get is that something might not work, and a note in the dev console.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't even realised CSS variables were that 'new'! (2014, but still)

A

@JelleZijlstra JelleZijlstra merged commit 50e5de2 into python:main Jan 16, 2022
@JelleZijlstra
Copy link
Member

Thanks @AA-Turner!

Speaking of merging things, would you be interested in joining the PEP editor team and receiving write access to this repository? You've been doing great work on this PEP and the rendering pipeline in general, and it'd be great to get more people on the team.

@AA-Turner
Copy link
Member Author

I'm honoured to be asked, aware it would be a big jump in responsibility!

I'd be very happy to -- let me know what information you'd need (credentials / evidence I'm a real person), or how best to proceed. I'll also need to read PEP 1 very carefully now :P

A

@AA-Turner
Copy link
Member Author

Also just noticed the build has failed with a mysterious error, just looking into that now - sorry.

xref https://github.com/python/peps/actions/runs/1705447524

@JelleZijlstra
Copy link
Member

I'm honoured to be asked, aware it would be a big jump in responsibility!

Great! No further credentials needed, your work speaks for itself. I don't have permissions myself to actually add you, but we'll hopefully do it as soon as we can locate someone who does.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Jan 16, 2022

Hugo especially, and Éric and Pablo contributed ideas (maybe code, but I can't remember) during the initial implementation stages. I'm not sure if everyone is required to declare their contributions to be PD, though

As a disclaimer, I'm not a software copyright attorney (I've only worked with one) and this is not legal advice.

In general, under national and international copyright law, it is not possible to copyright ideas, only expressions of them. What this means in practice is that code is copywritable, but the ideas leading to it are not, so if you were the only one who made substantial (more than a few lines) direct contribution to the literal text of the code, which appears to be reflected at least in the Git record, you're the only one with rights to release/disclaim.

I have a limited at best grasp of UK copyright law, let alone American!

UK law has a concept of public domain pretty similar to US law, though there are differences in the details (and the fact that the works of your Government have crown copyright and are not public domain and freely usable like ours are). However, it is good to ask, since that's the reason that the CC0 is used instead of just saying "this is public domain", since how something is released to it differs between jurisdictions, and many don't have a similar concept to PD at all (or it is very different/limited).

I'm honoured to be asked, aware it would be a big jump in responsibility!

Yay, really happy to see this! I was surprised you weren't already, given your extensive work on this!

Great! No further credentials needed, your work speaks for itself. I don't have permissions myself to actually add you, but we'll hopefully do it as soon as we can locate someone who does.

@gvanrossum ?

@gvanrossum
Copy link
Member

+1 on adding @AA-Turner as a PEP editor. (Who can make it happen? @brettcannon ?)

@AA-Turner
Copy link
Member Author

it is not possible to copyright ideas, only expressions of them

Yep, this is why the me not remembering who actually contributed code that I used is slightly embarrasing. It would be in the comments in the various original PRs, though.

A

@JelleZijlstra
Copy link
Member

@AA-Turner just checking, would you still be interested in working on PEP editing if PEP 676 were to be rejected? (To be clear, I think that's unlikely.)

@AA-Turner
Copy link
Member Author

@JelleZijlstra yes of course, whilst I would like PEP 676 to be approved I certainly will support things regardless of the outcome. Thanks for asking, though!

A

@hugovk
Copy link
Member

hugovk commented Apr 24, 2022

Good news!

I sent a PR to the upstream Pygments library to adjust the colours to meet WCAG AAA contrast guidelines: pygments/pygments#2038

Merged in January and just released: https://github.com/pygments/pygments/releases/tag/2.12.0

The new version will be used in our next deploy. To compare, I redeployed my fork and ran the WAVE Web Accessibility Evaluation Tool on PEP 8, and we're down from 137 contrast errors to none :)

Before

137 Contrast Errors

image

https://wave.webaim.org/report#/https://python.github.io/peps/pep-0008/

image

https://python.github.io/peps/pep-0008/

After

0 Contrast Errors

Congratulations! No errors were detected! Manual testing is still necessary to ensure compliance and optimal accessibility.

image

https://wave.webaim.org/report#/https://hugovk.github.io/peps/pep-0008/

image

https://hugovk.github.io/peps/pep-0008/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants