Change settings sidebar colors #9084
Conversation
|
@gtsiolis: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Thanks! Taking a look 👀 |
There was a problem hiding this comment.
Many thanks @gtsiolis for watching over accessibility, colors, and the dark theme! 🖤 ✨
While reviewing this Pull Request, I wonder whether this might be a duplicate of #9052 (where I noticed that a text color class of the active element was accidentally missing the dark: prefix). If so, then the problem might already be fixed on the main branch (not deployed to production yet):
| @@ -31,7 +31,7 @@ export function PageWithSubMenu(p: PageWithSubMenuProps) { | |||
| if (e.link.some((l) => l === location.pathname)) { | |||
| classes += " bg-gray-300 text-gray-800 dark:bg-gray-800 dark:text-gray-50"; | |||
| } else { | |||
| classes += " hover:bg-gray-100 dark:hover:bg-gray-800"; | |||
| classes += " hover:bg-gray-100 dark:hover:bg-gray-800 dark:text-gray-500"; | |||
There was a problem hiding this comment.
While attempting to test this change, I saw no difference between:
- https://gt-change-4c330c918c.staging.gitpod-dev.com/account
- and https://main.staging.gitpod-dev.com/account
I believe the reason is that, when you look at the combined classes, text is already gray-500 (see text-gray-500 in the common classes on line 28), so adding dark:text-gray-500 to these classes makes no difference.
|
@jankeromnes You're right, this is a duplicate of #9052 which I missed while being offline, see relevant discussion (internal). The issue seems to be already resolved in staging. Sorry for wasting your time and let me close this. 🍵 |
Description
Following the changes in #9002, this will change settings sidebar colors. 🖍️
How to test
/account.