FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Use different colour for 'Return value: Borrowed reference' by hugovk · Pull Request #188 · python/python-docs-theme · GitHub

  1. python
  2. python-docs-theme

Repository navigation

Use different colour for 'Return value: Borrowed reference' - #188

Merged
hugovk merged 2 commits into
python:mainfrom
hugovk:refcount-return-borrowed-ref
Apr 26, 2024
ConversationCommits (2)ChecksFiles changed
Merged

Use different colour for 'Return value: Borrowed reference'#188
hugovk merged 2 commits into
python:mainfrom
hugovk:refcount-return-borrowed-ref

Conversation

hugovk commented Apr 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Follow on from python/cpython#117926.

Currently light green is used for all return types:

  • "Return value: Always NULL."
  • "Return value: New reference."
  • "Return value: Borrowed reference."

For example: https://docs.python.org/3/c-api/exceptions.html

A borrowed reference is not necessarily a "bad" thing, but we may want to use something other than green that indicates it's "good", to indicate they should be treated with care.

This PR uses the same yellow/orange colour as the "Changed in version x.y" text in #185 - I've already checked the contrast ratios in that PR and both meet WCAG AAA. By using the same colour, it will be easier to check/change when we audit.

For example: https://python-docs-theme-previews--188.org.readthedocs.build/en/188/c-api/exceptions.html#querying-the-error-indicator

hugovk mentioned this pull request Apr 17, 2024
Merged

vstinner commented Apr 17, 2024

Copy link
Copy Markdown
Member

This PR uses the same yellow/orange colour as the "Changed in version x.y" text in #185

FYI As a color blind person, I don't see the difference between "Borrowed reference" and "New reference" colors in the light mode. Well, I'm also color blind, so my opinion may not matter :-)

In the dark mode, I see clearly that the two colors are very different.

flying-sheep commented Apr 19, 2024

Copy link
Copy Markdown

If you’re reusing existing colors, how about you centralize their definition as a CSS variables and then use it from both your new CSS rules and the old ones that originally defined the color?

hugovk commented Apr 19, 2024

Copy link
Copy Markdown
Member Author

Good idea, shall we also replace the pale green for the other "Return value" with the "New in version" green?

The light mode ones are much closer, but we could replace one with the other here too?

hugovk commented Apr 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Updated:

Suggestions for better variable names welcome :)

erlend-aasland approved these changes Apr 24, 2024
View reviewed changes

erlend-aasland left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't have a strong opinion regarding naming, but I like the change! Personally, I would prefer borrowed references to be labelled as "bad", but that is a different discussion.

hugovk merged commit 97952b5 into python:main Apr 26, 2024
hugovk deleted the refcount-return-borrowed-ref branch April 26, 2024 10:26

hugovk commented Jun 26, 2024

Copy link
Copy Markdown
Member Author

This is now deployed: https://docs.python.org/3/c-api/exceptions.html

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

erlend-aasland erlend-aasland approved these changes
encukou Awaiting requested review from encukou
vstinner Awaiting requested review from vstinner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL