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

Fixed #34910 -- Improved color contrast for add/change icons in admin. #17744

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

Hisham-Pak
Copy link
Contributor

@felixxm felixxm requested review from thibaudcolas and a team January 17, 2024 04:39
Copy link
Sponsor Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Thank you for giving this a go @Hisham-Pak, could you confirm what you’ve done to test this?

In particular I believe this needs further work for support of the admin’s dark theme – have you tested that?

@Hisham-Pak
Copy link
Contributor Author

Hi @thibaudcolas, thank you for your review. I have visually inspected the color and also looked for its occurrences elsewhere (didn't find any issues). It has passed the contrast test as well (although contrast ratio did decrease a little bit on dark mode). If there’s anything I might have overlooked, please let me know. Here’s how the changes appear in the current context:

Light Mode
light mode

Dark Mode
dark mode

Copy link
Sponsor Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Thank you @Hisham-Pak, my bad! This looks spot on, tested in light and dark mode though obviously the contrast grid you provided is enough to validate the change. Thank you for providing the screenshots in addition.

@felixxm felixxm removed the bug label Jan 19, 2024
@felixxm
Copy link
Member

felixxm commented Jan 19, 2024

@thibaudcolas Thanks for the review 👍 We don't use labels to categorize PRs, you don't need to add them.

@felixxm felixxm changed the title Fixed #34910 -- Color Contrast Admin Plus Icon And Admin Change Icon Fixed #34910 -- Improved color contrast for add/change icons in admin. Jan 19, 2024
@felixxm felixxm added the selenium Apply to have Selenium tests run on a PR label Jan 19, 2024
@felixxm
Copy link
Member

felixxm commented Jan 19, 2024

@Hisham-Pak Thanks 👍 Welcome aboard ⛵

@felixxm felixxm merged commit 8a1727d into django:main Jan 19, 2024
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
selenium Apply to have Selenium tests run on a PR
Projects
None yet
3 participants