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(updatenotification): a11y of channel menu and new icons #43746

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Feb 21, 2024

Summary

A11y fix

Before there were custom attributes for menuitemradio which were not fully correct.

  • button was inside menuitemradio but has to be menuitemradio itself
  • aria-label is not needed
  • aria-checked should be on the same element as menuitemradio

Since NcActionButton now supports radio out of the box, use it to have correct a11y attributes and consistent styles.

Before After
image image

Icons update

Check icon on Stable doesn't look good together with Check on checked variant.

Before After
image image

Replaced with Check in clouds. Also added icons for Daily and Git channels.

Daily Git
image image

Checklist

@ShGKme ShGKme added this to the Nextcloud 29 milestone Feb 21, 2024
@ShGKme ShGKme self-assigned this Feb 21, 2024
@ShGKme ShGKme force-pushed the fix/updatenotification--actions-a11y branch 2 times, most recently from 752303b to 6a73e97 Compare February 21, 2024 22:26
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Code looks clean!

@ShGKme ShGKme force-pushed the fix/updatenotification--actions-a11y branch from 6a73e97 to 6c1da91 Compare February 23, 2024 13:22
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 23, 2024
@skjnldsv skjnldsv force-pushed the fix/updatenotification--actions-a11y branch from 6c1da91 to fd4ced0 Compare February 23, 2024 13:40
@ShGKme ShGKme merged commit a88c1bd into master Feb 23, 2024
97 checks passed
@ShGKme ShGKme deleted the fix/updatenotification--actions-a11y branch February 23, 2024 14:31
@ShGKme
Copy link
Contributor Author

ShGKme commented Feb 23, 2024

/backport to stable28

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@ShGKme small detail: The icons seem to have gone from top-aligned with the headings (as intended design-wise) to center-aligned. Could that be fixed back? :)

@susnux
Copy link
Contributor

susnux commented Feb 28, 2024

@jancborchardt
Copy link
Member

@susnux ah thanks! :)

@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants