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(settings): emails actions a11y and design #43944

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Mar 1, 2024

Summary

  • Revert, a part of refactoring of FederationActions.vue from fix: Use nextcloud-vue components for personal info settings #43488
    • NcActions doesn't support having non-NcAction* as direct children which causes a11y issues
  • Change the design of additional emails and their actions, close to the old design
    • Use 2 buttons instead of 2-level menu
    • Show "This address is not confirmed" as input description instead of a caption in a menu
    • Add a gap between additional emails, otherwise they overlaps
    • Remove a margin on the right after actions
    • Renamed label "Additional email address N" with "Additional email N"

Screenshots

All screenshots are taken on the layout with the smallest width of inputs

Old design Before After
emails-old emails-before emails-new

After design discussions:

image

Checklist

@susnux
Copy link
Contributor

susnux commented Mar 1, 2024

NcActions doesn't support having non-NcAction* as direct children which causes a11y issues

Then we either should throw / show an error when non NcAction* components are used or fix the NcActions component to properly handle those.
Otherwise this sounds like a hack.

Use 2 buttons instead of 2-level menu

Is this accessibility related?

@nextcloud/designers whats your opinion here? Asking because for me it looks odd that the secondary email input fields now are a different width than the primary email.

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 1, 2024

Then we either should throw / show an error when non NcAction* components are used

Yep, though about it, should be added

or fix the NcActions component to properly handle those. Otherwise this sounds like a hack.

I don't see any reliable way to fully fix this case, except quite complex solutions. Having several days before the final a11y testing and only a single place with this issue I'd prefer to go with a simple solution in place.

Is this accessibility related?

Both.

From a design perspective, I'd try to avoid multi-level menus for simplicity and having a short way to the required action. It makes sense in a large menu, but here we have only 1-2 actions outside of the federation scope.

From an a11y perspective, I'm not sure. Neither submenu button nor the back button has anything except a text telling the user that it changes the menu, and I don't know if it is fine or not.

For example, in Files "Set reminder" has a bit broken focus behavior and no labeling that "Set reminder" on submenu is actually a Back button.

In this place, for me, NVDA doesn't react on moving to the submenu and back until I press an arrow key.

In general, I'd say we should have an interface to provide submenus out of the box in NcActions.

@nextcloud/designers whats your opinion here? Asking because for me it looks odd that the secondary email input fields now are a different width than the primary email.

We can also make the first input shorter 😀

Or use label outside for additional emails same as other labels on this page. Then scope federation menu can be on the label.

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 1, 2024

Or use label outside for additional emails same as other labels on this page. Then scope federation menu can be on the label.

Draft:

image

@susnux
Copy link
Contributor

susnux commented Mar 1, 2024

Or make the federation scope the trailing button? Then all input fields have the same width.

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 1, 2024

Or make the federation scope the trailing button? Then all input fields have the same width.

But we cannot put NcActions to the trailing button, NcInputField slot is only for an icon

@ShGKme ShGKme force-pushed the fix/settings--profile-federation-actions branch from 7c3535f to 32dec51 Compare March 1, 2024 22:41
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.

Or make the federation scope the trailing button? Then all input fields have the same width.

The federation scope looks better next to the label, and also makes more sense next to it as it is closer to its description.


@ShGKme I would be fine with your draft too. If accessibility-wise it is fine to have the button floating inside the input (on the right) like in the old design, then we could do that?
It would accomodate the actions, and ensure that all fields have the same width. It is also commonly done with e.g. password input visibility buttons.

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 4, 2024

If accessibility-wise it is fine to have the button floating inside the input (on the right) like in the old design, then we could do that?

It is a problem implementation-wise with our input fields.

First, we can only specify an icon for the trailing button but not the button. So we cannot put an actions menu there.

Second, on error/success state there is always an icon.

image

All these problems are solvable, but with dirty hacks.

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 4, 2024

@jancborchardt Is it fine?

image

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.

Yeah, that looks great in fact! :) The error icon was just duplicated so good on removing it.

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.

Designers said looks good, code looks fine, so 👍 😄

@ShGKme ShGKme force-pushed the fix/settings--profile-federation-actions branch from 79336d0 to ae65c34 Compare March 5, 2024 13:25
@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 5, 2024

  • Rebased onto master
  • Squashed fixup commits
  • A small change - keep original input top padding to be consistent with all of the other inputs
  • A small change - moved styles with margin between additional emails from Email component to EmailSection component

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.

Tested, and is much nicer!

@Pytal
Copy link
Member

Pytal commented Mar 5, 2024

Cypress is related

@ShGKme ShGKme force-pushed the fix/settings--profile-federation-actions branch from ae65c34 to 980316c Compare March 5, 2024 16:57
Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme ShGKme force-pushed the fix/settings--profile-federation-actions branch from 980316c to 7a8c581 Compare March 5, 2024 21:12
@skjnldsv skjnldsv disabled auto-merge March 5, 2024 21:32
@skjnldsv skjnldsv merged commit 9f1f123 into master Mar 5, 2024
97 of 98 checks passed
@skjnldsv skjnldsv deleted the fix/settings--profile-federation-actions branch March 5, 2024 21:32
@skjnldsv
Copy link
Member

skjnldsv commented Mar 5, 2024

Thanks for this @ShGKme !! 🙇 🤗

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 5, 2024

/backport 680f439 to stable28

Copy link

backportbot bot commented Mar 5, 2024

The backport to stable28 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable28
git pull origin stable28

# Create the new backport branch
git checkout -b backport/43944/stable28

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 680f439

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/43944/stable28

Error: No changes found in backport branch


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 5, 2024

Backport is not needed, at least, unless #43488 is backported

@blizzz blizzz mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants