Show member popover when user avatar selected #2999
Conversation
|
|
There was a problem hiding this comment.
Very nice work! 💯
Congratulations on your first PR!
This is all good, I just rejected it because I want you to add the "fancy' popover in one more place: Annotations on the Actions/Motions pages
See this screenshot:
The user widget on the right has the correct member popover (the one that you just added in this PR), while the one that appears over the avatar in Annotations, is still the "basic" version.
Please add it in there as well.
PS: you can drop the "Feature" from your PR's title, we only use "tags" like that for commit messages, PR titles should be more "human friendly" :)
| <div | ||
| /* | ||
| * Clicking on UserAvatar would redirect to Actions page and stop | ||
| * interaction with popover. | ||
| * stopPropagation prevents event being inherited by child | ||
| */ | ||
| onClick={stopPropagation} | ||
| onKeyPress={stopPropagation} | ||
| role="button" | ||
| tabIndex={0} | ||
| className={styles.avatar} | ||
| > |
There was a problem hiding this comment.
I'm good with this change, however there's an handleOnClick prop that's coming in, which assumes it can run the click action on the whole item.
Stopping propagation on the avatar, might impede that to some extent.
But since there's not "proper" way to check the execution requirements of the onClick function that's coming in (well, at least not without going a very deep dark rabbit hole), it will have to do for now
There was a problem hiding this comment.
The onClick was being passed to MemberInfoPopover, so it became clickable (same as the parent listItem), taking the user to the DefaultAction page.
Preventing the propagation allows MemberInfoPopover to behave as expected.
src/modules/core/components/DetailsWidgetUser/DetailsWidgetUser.tsx
Outdated
Show resolved
Hide resolved
src/modules/core/components/InfoPopover/MemberInfoPopover/MemberInfoPopover.css
Outdated
Show resolved
Hide resolved
danbr
changed the title
There was a problem hiding this comment.
All good now, however there's a "rogue" merge commit in your commit history for this branch that shouldn't be there (most likely a rebase gone wrong)
Please clean up your commit history, after which you can merge
I'm pre-approving in the mean time.
(PS: let me know if you have trouble merging due to permissions issues)
…r.tsx Agreed. Also in general, I wondered when the 'showArrow' is used? (connecting the avatar with the popover) Co-authored-by: Raul <rdig@users.noreply.github.com>
danbr
force-pushed
the
feat/2847-actionpage-show-member-popover
branch
from
e300bbf to
dceec2e
Compare
Description
This PR updates Actions, Events, Extension, & Motion pages to show user information popover when clicking on UserAvatar.
Bugs
Some issues that were identified and corrected along the way:
Resolves #2847