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

Fix color contrast in header and footer #1233

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xi
Copy link

@xi xi commented Oct 11, 2022

This replaces #998. I decided to create a new pull request because the code had changed so much in the meantime that a simple rebase was not enough.

I noticed that the dark theme does not inverse colors in the header and footer, but improves their contrast. So my proposal is to use the better contrast version with both themes.

I did not check the contrast on other parts of the page.

@xi xi mentioned this pull request Oct 11, 2022
@pauloxnet
Copy link
Member

This replaces #998

Thanks for opening a new PR.

I did not check the contrast on other parts of the page.

So other parts of the page will be checked in other PRs?

To help to review this PR can you add details about new contrast ratios as you did in the previous PR and one or more screenshots?

@xi
Copy link
Author

xi commented Oct 12, 2022

Screenshot

I did not check the contrast on other parts of the page.

So other parts of the page will be checked in other PRs?

That would be significant work. My previous pull request that changed only 7 lines stayed open for more than two years. I would be happy to work on that, but only if there is a clear commitment from the django project.

@xi
Copy link
Author

xi commented Oct 12, 2022

In #998 I had to change some brand colors to make this work. So I had to come up with a target contrast ratio and modify the colors to meet that goal. This work was already done in #1213 for the dark theme. All I did now was to apply the improved colors from the dark theme also to the light theme.

@knyghty
Copy link
Member

knyghty commented Oct 12, 2022

@thibaudcolas hopefully you can find time to take a look.

@pauloxnet
Copy link
Member

As @thibaudcolas highlited in the closed PR for all other text, we need to aim for 4.5:1 at a minimum.

In your changes could you use the color variables (e.g. var(--primary)) instead of the colors directly (e.g. $green-medium) ?

@xi
Copy link
Author

xi commented Oct 12, 2022

In your changes could you use the color variables (e.g. var(--primary)) instead of the colors directly (e.g. $green-medium) ?

I intentionally didn't use the custom properties because they change with the theme, while my proposal here is to keep the same colors for both themes. I could restructure the code to use custom properties to the same effect if that is desired.

@ghost
Copy link

ghost commented Nov 29, 2022

As the last update in this conversation was on October 12, I would like to ask @xi if this pull request is still in progress, as I was interested in making a PR regarding light-mode accessibility.

@xi
Copy link
Author

xi commented Nov 29, 2022

From my POV this is done. It is on the maintainers to decide whether they want this or not. If they decide they want to merge this I can fix any merge conflicts that have accumulated over time. But I don't really have the time to keep up with the main branch all the time.

@ghost
Copy link

ghost commented Nov 29, 2022

Makes sense, thanks for the response.

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

Successfully merging this pull request may close these issues.

4 participants