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.

Sign up for GitHub

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

Jump to bottom

[user_status] Improve user online status modal #38127

Merged
merged 6 commits into from
May 15, 2023
Merged

[user_status] Improve user online status modal #38127

merged 6 commits into from
May 15, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented May 8, 2023 •
edited
Loading

Summary

  1. Fix the predefined statuses buttons to implement a role (radio)¹ instead of being just tabable divs
    • Also set role to radiogroup of list container to group the predefined statuses
    • ¹Use native radio buttons
  2. Group custom message input elements and assign label to emoji selector
  3. Add missing radiogroup role to online status selector

TODO

For 1: I have also created a commit where I use real radio buttons (in the background) for the predefined statuses, so the question is should I push it here too? Do we want native radio buttons instead? What do you think @Pytal @JuliaKirschenheuter ?

Checklist

@susnux susnux added 3. to review Waiting for reviews accessibility labels May 8, 2023
@susnux susnux requested review from JuliaKirschenheuter, Pytal, a team, Fenn-CS and szaimen and removed request for a team May 8, 2023 15:21
@szaimen szaimen removed their request for review May 8, 2023 15:41
@szaimen szaimen added this to the Nextcloud 27 milestone May 8, 2023
@Pytal
Copy link
Member

Pytal commented May 8, 2023

Let's go with real radio inputs @susnux as it's the recommended way https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/radio_role#sect1 :)

This was referenced May 9, 2023
Closed
Merged
@susnux
Copy link
Contributor Author

susnux commented May 9, 2023

@Pytal OK done (see last commit)

@susnux susnux force-pushed the fix/a11y-status-modal branch 2 times, most recently from 3e80bc8 to 91edc9c Compare May 11, 2023 11:23
@susnux susnux requested a review from Antreesy May 11, 2023 11:25
@susnux
Copy link
Contributor Author

susnux commented May 11, 2023

/compile

@susnux susnux requested a review from nickvergessen May 11, 2023 12:58
@nickvergessen
Copy link
Member

Can we have before and after screenshots?

Antreesy
Antreesy approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

image

No visual changes (before => after), except for one:

  • when selected 'button' is not in focus, it now keep selected styles

No blocking changes, except for redundant class

apps/user_status/src/components/PredefinedStatus.vue Outdated Show resolved Hide resolved
apps/user_status/src/components/PredefinedStatus.vue Outdated Show resolved Hide resolved
Antreesy
Antreesy requested changes May 11, 2023
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

And please rebuild

@susnux susnux force-pushed the fix/a11y-status-modal branch 2 times, most recently from 2f32b69 to 25dcb9c Compare May 11, 2023 14:28
@susnux
Copy link
Contributor Author

susnux commented May 11, 2023

/compile

@susnux susnux requested a review from Antreesy May 11, 2023 14:28
Antreesy
Antreesy approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

🦭

@Antreesy Antreesy enabled auto-merge May 11, 2023 15:55
@susnux
fix(user_status): Use role=radio for predefined statuses in online st…
2133b0e 
…atus modal

* Instead of tabable DIVs properly assign the radio role
* Set role to radiogroup of list container to group the predefined statuses

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
susnux and others added 5 commits May 15, 2023 14:31
@susnux
fix(user_status): Group custom message input elements and assign labe…
a99265b 
…l to emoji selector

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
fix(user_status): Add missing radiogroup role to online status sele…
349e12e 
…ctor

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
fix(user_status): Use native radio buttons for online status selection
e93b20c 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
fix(user_status): Remove redundant styling, use global class instead
8506d90 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@nextcloud-command @susnux
Compile assets
c588c1d 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@susnux
Copy link
Contributor Author

susnux commented May 15, 2023

rebased

@Antreesy Antreesy merged commit 77efa2e into master May 15, 2023
@Antreesy Antreesy deleted the fix/a11y-status-modal branch May 15, 2023 20:01
@blizzz blizzz mentioned this pull request May 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pytal Pytal Pytal approved these changes

@Antreesy Antreesy Antreesy approved these changes

@JuliaKirschenheuter JuliaKirschenheuter Awaiting requested review from JuliaKirschenheuter

@Fenn-CS Fenn-CS Awaiting requested review from Fenn-CS Fenn-CS was automatically assigned from nextcloud/server-frontend

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees
No one assigned
Labels
3. to review Waiting for reviews accessibility
Projects
None yet
Milestone
Nextcloud 27
6 participants
@susnux @Pytal @nickvergessen @Antreesy @szaimen @nextcloud-command