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

MPDX-8217 change the contact and task row links into actual links. #1090

Merged
merged 23 commits into from
Oct 23, 2024

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Sep 24, 2024

Description

In this PR, I converted the contact and task JS click functions into actual links that can be opened in a new tab. This helps with UX

Contact pages
List view: the contacts on the list view are now links
Flows view: The contacts on the flows view are now links.
Map view: the contacts are not links.
Contact details: The links to other contacts on a contact detail page are now links

Task Page
List view: the contacts on the list view are now links.

Reports
All the report pages that feature contacts names are now links.

Coaches
There is no need to edit anything here.

Tools
Appeals: I have made the appeal contacts names into links on both list and flows view

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@dr-bizz dr-bizz requested a review from canac September 24, 2024 18:05
Copy link
Contributor

github-actions bot commented Sep 24, 2024

Bundle sizes [mpdx-react]

Compared against 0b3c384

No significant changes found

Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you moved some of the logic into a custom hook 🔥

src/components/Contacts/ContactRow/ContactRow.tsx Outdated Show resolved Hide resolved
src/components/Contacts/ContactsMap/ContactsMap.tsx Outdated Show resolved Hide resolved
@@ -42,7 +42,7 @@ export const FourteenMonthReport: React.FC<Props> = ({
currencyType,
isNavListOpen,
title,
onSelectContact,
getContactUrl,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of prop-drilling getContactUrl down from the parent, what do you think about using the useGetContactUrls hook in this component and passing in the path based on the currency type? I can see a case for leaving it as-is, so let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep the controls in one place and since it's only going down 1 component, I think we are okay

src/components/Task/TaskRow/TaskRow.tsx Outdated Show resolved Hide resolved
src/components/Task/TaskRow/TaskRow.tsx Outdated Show resolved Hide resolved
src/components/Tool/Appeal/List/ContactRow/ContactRow.tsx Outdated Show resolved Hide resolved
src/hooks/useContactLinks.ts Outdated Show resolved Hide resolved
@canac
Copy link
Contributor

canac commented Sep 24, 2024

Functionality looks really good too! One thing I noticed is that in task rows, the contact's name is always underlined now instead of just being underlined on hover. Also, command+clicking contacts and tasks to open them in a new tab opens the contact or task in the tab I was in and opens them in a new tab. Normally command+click doesn't change the current URL.

@dr-bizz dr-bizz requested a review from canac October 21, 2024 19:12
@canac canac added the Preview Environment Add this label to create an Amplify Preview label Oct 21, 2024
@dr-bizz
Copy link
Contributor Author

dr-bizz commented Oct 22, 2024

Even more conflicts 😭

@canac canac added Preview Environment Add this label to create an Amplify Preview and removed Preview Environment Add this label to create an Amplify Preview labels Oct 22, 2024
Copy link
Contributor

Preview branch generated at https://MPDX-8217.d3dytjb8adxkk5.amplifyapp.com

…chPolicy: 'standby',` was causing issues on local
@dr-bizz dr-bizz requested a review from canac October 22, 2024 18:27
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only issue I'm seeing is links always being underlined instead of only being underlined on hover, so I'm going to approve.

We might want to add defaultProps: { underline: 'hover' } to MuiLink in the theme to ensure consistency in all links across the application. That could also be done later.

@dr-bizz dr-bizz merged commit acc4107 into main Oct 23, 2024
17 of 18 checks passed
@dr-bizz dr-bizz deleted the MPDX-8217 branch October 23, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants