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 7948 - Creating appeal details pages #984

Merged
merged 18 commits into from
Aug 22, 2024

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Jul 30, 2024

Description

This PR creates the Appeals Detail page structure, including the both List and Flows views. The page setup allows opening a contact drawer within the appeal details page and includes basic List and Flows pages with filtering functionality.

To make reviewing this PR easier, I have split it up into 5 different stages.

1. (Set up Appeal Detail pages)

In this step, I create the Appeal page and set it up to open 4 different types of pages. Initial appeal page, detailed list view, detailed flows view and a contact view.

2. (Clean up old files)

In this step, I remove old files and move the initial files into a folder.

3. (Appeal Details Frame )

In this step, I build out the List view, which includes the ListHeader, HeaderInfo, and some other components that are reused in the Flows view.
This step also includes the list infinite scroll and ContactRow component changes.

4. (Flows View)

In this step, I build out the Flows view, building the columns and drag and drop functionality. In this step and step 4 I try to reuse the contacts components as much as possible to prevent duplication. You will see I've edited and exported styled components & functions from the sibling contact components.

5 (Fixing issues)

In this step, I fix a few errors.

Changes

  • Updated [appealId] page to [[...appealId]] for comprehensive path capturing.
  • Implemented functionality to open a contact within an appeal detail page.
  • Created basic List and Flows pages.
  • Added filters for flows and list views.
  • Removed obsolete files.

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

Copy link
Contributor

github-actions bot commented Jul 30, 2024

Bundle sizes [mpdx-react]

Compared against 02e118e

Route Size (gzipped) Diff
/accountLists/[accountListId]/tools/appeals/appeal/[[...appealId]] 210.2 KB added
/accountLists/[accountListId]/tools/appeals/[appealId] no change removed
Dynamic import Size (gzipped) Diff
../src/components/Contacts/ContactFlow/DynamicContactFlow.tsx -> ./ContactFlow 54.5 KB +13.49 KB
../src/components/Contacts/ContactsList/DynamicContactsList.tsx -> ./ContactsList 38.38 KB +9.43 KB
../src/components/Contacts/ContactsRightPanel/DynamicContactsRightPanel.tsx -> ./ContactsRightPanel 139.52 KB +3.8 KB
../src/components/Shared/Filters/DynamicFilterPanel.tsx -> ./FilterPanel 104.47 KB +3.79 KB
../src/components/Tool/Appeal/Flow/DynamicContactFlow.tsx -> ./ContactFlow 54.5 KB added
../src/components/Tool/Appeal/List/AppealsListFilterPanel/DynamicAppealsListFilterPanel.tsx -> ./AppealsListFilterPanel 2.57 KB added
../src/components/Tool/Appeal/List/ContactsList/DynamicContactsList.tsx -> ./ContactsList 38.38 KB added

@dr-bizz dr-bizz requested review from caleballdrin and removed request for wrandall22 August 1, 2024 20:41
@caleballdrin caleballdrin added the Preview Environment Add this label to create an Amplify Preview label Aug 1, 2024
Copy link
Contributor

github-actions bot commented Aug 1, 2024

@caleballdrin
Copy link
Contributor

When you select or add a new contact to an appeal, it should be put in the 'Asked' category.

Copy link
Contributor

@caleballdrin caleballdrin left a comment

Choose a reason for hiding this comment

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

This is a huge page. Great work on everything! I've just got a couple of changes on things that don't seem to be working right.

>
<Box display="flex" flexDirection="column" justifyContent="center">
<Typography component="span">
{`${pledge} ${frequency}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like sometimes it is not showing the actual commitment like it did in Angular. I'm looking at John Doe Lois Lane's account and the End of Year Gift appeal.
Screenshot 2024-08-02 at 11 49 51 AM

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 am asking Andrew about this as I'm not able to grab the given total without doing a separate request for each

Copy link
Contributor Author

@dr-bizz dr-bizz Aug 21, 2024

Choose a reason for hiding this comment

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

I Will be fixing this in a later PR

@dr-bizz dr-bizz requested a review from caleballdrin August 21, 2024 19:14
@@ -105,7 +105,7 @@ const Appeal = ({
className={classes.nameLink}
>
<NextLink
href={`/accountLists/${accountListId}/tools/appeals/${id}`}
href={`/accountLists/${accountListId}/tools/appeals/appeal/${id}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need /appeal/ here? It seems a little repetitive.

Copy link
Contributor Author

@dr-bizz dr-bizz Aug 21, 2024

Choose a reason for hiding this comment

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

Yes, due to the flashing appeal initial page issue. To prevent the flashing initial page issue, we need to make it into a separate page.

@dr-bizz dr-bizz requested a review from canac August 21, 2024 20: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 code looks really good! Now I need to test the functionality out, but here are a few comments from just looking at the code.

@canac
Copy link
Contributor

canac commented Aug 21, 2024

Now that #999 is merged, you could use the autoscroll hook I made for contact flows in appeals at some point. A follow-up PR would be fine.

@canac
Copy link
Contributor

canac commented Aug 21, 2024

Oh also, there's a lot of new tests that use waitFor and getBy.... Personally, I prefer to just use await findBy because it's more concise and results in less code noise in the tests. I didn't flag them all, and I'm not going to say you have to change them, but it's something to be aware of in case you want to fix them and/or for future test code.

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Aug 22, 2024

@canac I will do both of the edits you mention in another PR, as I don't want to screw up my commit history too much.

@dr-bizz dr-bizz requested a review from canac August 22, 2024 17:46
@canac
Copy link
Contributor

canac commented Aug 22, 2024

You might be aware of this, but I'm getting GraphQL errors when attempting to move a contact in the flows view.

Screenshot 2024-08-22 at 1 05 31 PM

And the goal amount doesn't seem to load. It stays stuck on the skeleton.

Screenshot 2024-08-22 at 1 05 47 PM

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.

This is amazing work! I noticed a few bugs when I tried it out, but if you want to address those in future PRs, I'm good for this PR to be merged.

@dr-bizz dr-bizz dismissed caleballdrin’s stale review August 22, 2024 18:11

Caleb C reviewed

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Aug 22, 2024

And the goal amount doesn't seem to load. It stays stuck on the skeleton.

This get fixed in later PRs

@dr-bizz dr-bizz force-pushed the MPDX-7948-creating-appeal-details-pages-2 branch from c8577b5 to 43047d2 Compare August 22, 2024 18:27
@dr-bizz dr-bizz enabled auto-merge August 22, 2024 18:32
@dr-bizz dr-bizz merged commit 49b3b7d into main Aug 22, 2024
17 checks passed
@dr-bizz dr-bizz deleted the MPDX-7948-creating-appeal-details-pages-2 branch August 22, 2024 18:37
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.

3 participants