-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[PM-4871] Remove clickable row in favor of clickable cell content #6911
Conversation
Thank you for your contribution! We've added this to our internal Community PR board for review. |
New Issues
Fixed Issues
|
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.
This behavior seems to align with the Design request outlined in: PM-3078
@gbubemismith when you get to reviewing this for Vault please reference the internal task which outlines our desired fix for this UX bug.
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.
@Spitfireap Thanks for fixing this bug. I have one suggestion, and we can get this approved.
queryParamsHandling: "merge", | ||
}); | ||
@HostListener("click", ["$event.target"]) | ||
protected click(target: HTMLSpanElement) { |
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.
nitpick: I think the target parameter can be changed to HTMLElement
as it is a more general type.
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.
@gbubemismith Change committed ;).
The lint process applied some change on L.37. Do you want to keep them ?
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 for jumping on this quickly! I noticed something on a second look: we are shifting our UX to favour clickable cells instead of the entire row. Given this requirement, it looks like removing the HostListener
and the HostBinding
should meet our aim. Would you mind making those changes? I really appreciate your work on this.
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.
My pleasure and done ! Thanks for your review.
I've deleted L.37 altogether (activatedRouter
didn't seem to be used before). I'm not fluent enough in Angular to be sure it wasn't needed (even though I didn't see any regression). I would appreciate it if you could double-check.
On a side note I feel that making the clickable area for the checkbox wider than its visual appearance (as proposed initially) would still be an accessibility improvement...
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.
One last thing: can you fix the conflict on your branch?
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.
done.
|
Hello @Spitfireap, Firstly, thank you for your work on this bug. I moved this to our QA team to test and received feedback on adding this behaviour to |
@gbubemismith done :). |
Thanks for your quick response. I have started this discussion and am sure this suggestion will be considered. |
@Spitfireap Our QA team has approved this PR 🚀. Thanks for your work on this. |
Type of change
Objective
Improve UX.
When selecting multiple items in the vault it is easy to mis-click and trigger the "show item" action which not only show the item information but also uncheck every previously checked checkbox. It is really hard to manage a great number of items.
The proposed change check the click event and if the mouse is above the checkbox's
<td>
the checkbox will trigger instead.Forum link referring to this issue.
Code changes
<td>
containing the checkbox for the click event checkScreenshots
old behavior:
old.mp4
new behavior:
new.mp4