MPDX-7814 - Allow users to open contact in new tab #900
Conversation
Bundle SizesCompared against 5003e45 Route: No significant changes found Dynamic import: None found. |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
I guess you couldn't get it to work by making the list items actual Next <Link>s and then letting the browser natively handle opening links in a new tab?
Is it hard to get the context menu to show up under the cursor instead of anchored to the left edge of the contact row?
| @@ -153,4 +153,43 @@ describe('ContactsRow', () => { | |||
| userEvent.click(rowButton); | |||
| // TODO: Find a way to check that click event was pressed. | |||
| }); | |||
|
|
|||
| it('should NOT open menu on right-click', () => { | |||
There was a problem hiding this comment.
Is this what you meant?
| it('should NOT open menu on right-click', () => { | |
| it('should NOT open menu on left-click', () => { |
| const rowButton = getByTestId('rowButton'); | ||
| expect(queryByRole('menu')).not.toBeInTheDocument(); | ||
| userEvent.click(rowButton); | ||
| expect(queryByRole('menu')).not.toBeInTheDocument(); |
There was a problem hiding this comment.
In the other test, you are waiting for the menu to be visible. If the menu shows up asynchronously, then this test will pass even if the menu eventually shows up. Let's either remove the waitFor in the other test if that works or if not, add a waitFor to this test.
| return ( | ||
| <Menu | ||
| anchorEl={returnRightClickRef} | ||
| open={showMenu} |
There was a problem hiding this comment.
Is there a reason this doesn't work? Then you could get rid of returnRightClickRef and remove the TypeScript cast.
| open={showMenu} | |
| anchorEl={rightClickRef.current} |
| const { getByRole, queryByRole } = render(<Components />); | ||
|
|
||
| const button = getByRole('button'); | ||
| expect(button).toBeInTheDocument(); |
There was a problem hiding this comment.
Did you know that expect().toBeInTheDocument() is usually redundant? getByRole will fail the test if it isn't in the document. If you're just checking the existence of an element, I think it's fine to do expect().toBeInTheDocument(). But if you're interacting with an element (like clicking a button), I usually let the getByRole implicitly assert that the element exists and skipping the redundant .toBeInTheDocument() check. Feel free to disagree if you prefer the explicitness.
|
|
||
| interface Props { | ||
| contactId: string; | ||
| rightClickRef: React.MutableRefObject<null>; |
There was a problem hiding this comment.
I think a name like menuAnchorRef would be more descriptive.
pages/accountLists/[accountListId]/contacts/ContactsContext.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/contacts/ContactsContext.tsx
Outdated
Show resolved
Hide resolved
| contactId: string; | ||
| rightClickRef: React.MutableRefObject<null>; | ||
| showMenu: boolean; | ||
| setShowMenu: React.Dispatch<React.SetStateAction<boolean>>; |
There was a problem hiding this comment.
This component only needs to hide the menu, it never needs to setShowMenu(true), so what if we make this prop onHideMenu instead of exposing the setter?
|
@canac There could be a lot of useful quick actions under the right-click menu that could help the contacts be more productive, which is why I did the right-click menu. As someone who doesn't use MPDx, I might be wrong, do you feel like this would benefit the user or will adding a NextLink be fine and users don't need quick actions? If we use the right-click menu, it should show where you clicked. - I'll work on this. |
There's a lot of users of MPDX, so I'm sure some of them would find it useful. Personally, I'm more of a keyboard user, so shortcuts like Cmd+click to open in a new tab or keyboard shortcuts like "D" to open the contact details donations tab, "T" to open the contact details tasks tab, etc would speed up my workflow than a right-click menu. I'm probably in the minority though. I think most desktop users use the mouse a lot more than keyboard shortcuts. |
dr-bizz
force-pushed
the
MPDX-7814-right-click-contacts
branch
from
1040bbd to
c66e62b
Compare
Bundle sizes [mpdx-react]Compared against 115b5ac No significant changes found |
|
I have pushed this code live in another PR #1090 |
Description
https://jira.cru.org/secure/RapidBoard.jspa?rapidView=3&view=detail&selectedIssue=MPDX-7814#
Added a custom right-click menu, which for now only has the menu item
Open contact in new tab.But we could easily add quick actions to the right-click menu, making users make fewer clicks. One idea I had was for an open to allow the user to edit the contact details. which would open the contact details modal.
If users like the right-click, we could even use it on Tasks.
I also corrected some types in ContactContext
Checklist: