Skip to content

Conversation

vyncent-t
Copy link
Contributor

@vyncent-t vyncent-t commented Aug 5, 2025

Summary

This PR fixes the issue where the text for notification menu items were not readable on darker themes

Related Issue

Related to issue #3746

Changes

  • Added contrast text via theme.palette to all notification menu items
  • Keeps buttons disabled when needed, just not readable

Steps to Test

  1. Navigate to notifications
  2. Click on the menu button
  3. Note that they are now contrast friendly

Screenshots (if applicable)

Before

image image image

After

disabled

image image image

enabled

image image image

@vyncent-t vyncent-t requested a review from illume August 5, 2025 17:13
@vyncent-t vyncent-t self-assigned this Aug 5, 2025
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 5, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vyncent-t
Once this PR has been reviewed and has the lgtm label, please assign illume for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vyncent-t vyncent-t added a11y Accessibility related issues and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 5, 2025
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 5, 2025
@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 5, 2025
@vyncent-t vyncent-t marked this pull request as ready for review August 6, 2025 16:10
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2025
@k8s-ci-robot k8s-ci-robot requested a review from knrt10 August 6, 2025 16:10
@sniok
Copy link
Contributor

sniok commented Aug 8, 2025

But now the buttons don't look disabled, they look like you can click on them which is misleading.
Disabled items don't have contrast requirement [source]

@vyncent-t vyncent-t force-pushed the fix-notifications-contrast-menu branch from 50bfe8e to 53dbccd Compare August 20, 2025 18:30
@vyncent-t
Copy link
Contributor Author

But now the buttons don't look disabled, they look like you can click on them which is misleading. Disabled items don't have contrast requirement [source]

current contrast was too hard to read, tweaked the new contrast to still be faded and not solid, let me know if this is readable

@@ -104,11 +104,31 @@ export default function NotificationList() {
<Icon icon="mdi:dots-vertical" />
</IconButton>
<Menu anchorEl={anchorEl} open={Boolean(anchorEl)} onClose={handleClose}>
<MenuItem onClick={markAllAsRead} disabled={!hasUnseenNotifications}>
<Typography color={'primary'}>{t('translation|Mark all as read')}</Typography>
Copy link
Contributor

Choose a reason for hiding this comment

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

setting color as <Typography color="text.primary">... fixes this
there's no need for any additional styles

 <MenuItem onClick={markAllAsRead} disabled={!hasUnseenNotifications}>
    <Typography color="text.primary">{t('translation|Mark all as read')}</Typography>
  </MenuItem>
Image Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that adjustment does work but only for the lights out contrast (it was blue before).

however for the dark mode contrast it is still the same contrast as the original issue #3746

before
image

after
image

@vyncent-t
Copy link
Contributor Author

vyncent-t commented Aug 21, 2025

Just to be clear I believe the main goal here is to make the disabled text pop a little bit more while still looking faded enough to not be mistaken as enabled.

If the current colors are still too solid and do not fit disabled styles enough I can adjust it to be lighter.

@vyncent-t vyncent-t force-pushed the fix-notifications-contrast-menu branch 3 times, most recently from c154ebe to f71a85c Compare August 25, 2025 18:16
@vyncent-t vyncent-t force-pushed the fix-notifications-contrast-menu branch from f71a85c to 748559e Compare August 26, 2025 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility related issues cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants