-
Notifications
You must be signed in to change notification settings - Fork 85
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(NcAvatar): Remove span wrapper button semantics in favour of internal button components #5131
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Chris for cleaning this up!
Works! One question: could we return a focus to the avatar (action button) when action menu is closed? Or is it another issue?
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both solutions proposed works, all just a matter of preference 👍
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…rnal button components Signed-off-by: Christopher Ng <[email protected]>
1ee2040
to
c94784c
Compare
Signed-off-by: Christopher Ng <[email protected]>
Signed-off-by: Christopher Ng <[email protected]>
Moved the button semantics to the actual buttons rather than the wrapper span while preserving the same UX as before (slightly strange NcActions click triggering toggleMenu to fetch the latest contactsmenu data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems legit, tested listeners and new template
☑️ Resolves
Summary
Previous iteration
🏁 Checklist