-
Notifications
You must be signed in to change notification settings - Fork 1
[MPDX-8973 MPDX-8974 MPDX-8981 MPDX-8982 MPDX-8987 MPDX-8988] - PGA Report Update #1479
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
base: main
Are you sure you want to change the base?
Conversation
Bundle sizes [mpdx-react]Compared against 88bb693
|
|
Preview branch generated at https://MPDX-8988-additional-pga-fields.d3dytjb8adxkk5.amplifyapp.com |
canac
left a comment
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 looks really good! I guess you're still working on making printing print all pages? And are you still working on adding the donor ID?
You grouped a lot of tickets into this PR. That's OK since they're small, but I would have also been OK with multiple tiny PRs. They're fast to test and approve.
Is it OK for this PR to merge into production since we don't have a production SAA server yet? No production users will have a staff account. So we might want to hide comment out the skeleton for now. Also, is Scott OK with these changes going live as soon as they are approved? Or does he want to do UAT first?
pages/accountLists/[accountListId]/reports/partnerGivingAnalysis/[[...contactId]].page.tsx
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/PartnerGivingAnalysisReport.tsx
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/PartnerGivingAnalysis.graphql
Outdated
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/BalanceCard/BalanceCard.test.tsx
Outdated
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/BalanceCard/BalanceCard.test.tsx
Outdated
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/PartnerGivingAnalysisReport.tsx
Outdated
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/Actions/HeaderActions.tsx
Show resolved
Hide resolved
|
I see now in the ticket that you said you're no longer adding the donor id to the report. Can you update the Jira ticket to reflect that? |
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 more minor comment. The code looked great while I was testing it!
src/components/Reports/PartnerGivingAnalysisReport/PartnerGivingAnalysisReport.test.tsx
Outdated
Show resolved
Hide resolved
|
@canac, let me check with Scott on merging this to production. The Let me know if you want me to break these up. That was my plan initially but after realizing how small some of these tasks were I decided to just put it all in one PR. |
@wjames111 Correct, but won't the skeleton show temporarily while loading and then disappear and cause layout shift since no production users have staff accounts yet? |
canac
left a comment
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.
Basically there, so I'll approve. Thanks for all of the improvements! Before merging, make sure to test that the double-scrolling is gone, and let's have a plan for the loading skeleton (I'm thinking we comment it out, but we can discuss).
src/components/Reports/PartnerGivingAnalysisReport/Actions/HeaderActions.tsx
Outdated
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/PartnerGivingAnalysis.graphql
Outdated
Show resolved
Hide resolved
src/components/Reports/PartnerGivingAnalysisReport/PartnerGivingAnalysisReport.tsx
Outdated
Show resolved
Hide resolved
|
@canac I'm having trouble recreating the double scrolling issue. I do see two scrollbars one for the page and one for the table, is this what you are talking about? If so do you know of a good fix? I could make the BalanceCard smaller but then it wouldn't be uniform to the other reports or I could reduce the size of the table but then that makes reading it a little more difficult. |
Yeah, that's it. I see what you're saying about keeping the same card. That balance card takes up a lot of vertical screen real estate though. It also makes it so that you have to scroll the page to the bottom of the table then scroll the table to get to the pagination controls. Is it a requirement that we use the same balance card? If not, put we put that information inline with the search bar (just a quick mockup, the padding and layout is a little off)?
As a user, I think I'd rather see a shorter balance card so that I can see more rows of the table. As a user, I personally don't care that much about the primary account balance because I probably just saw it on the dashboard page before navigating to the PGA report. So I would want it to be unobtrusive. In the current layout on my large laptop, the table only gets half of the vertical space. I'm not familiar with any requirements discussions though, these are just my own personal thoughts. If you want feedback from someone on the UX team, feel free to ping Ryan to see if he has ideas (not required though). If we need to use the same budget card, then we should probably remove On an unrelated note, I see the "total donation for this period" requirement in MPDX-8981? Where is that implemented? |
So the use of the current balanceCard was suggested by Joanna, I don't believe it was a requirement though, so I think we could probably do your inline suggestion. I forgot "total donation for this period" needs to be added to the card as well. This might cost us some space too, let me play around with the card and see if I can get something that fits. |
476cf83 to
078f9a3
Compare
b0a01f0 to
4d45795
Compare
…ates; remove unused props
4d45795 to
c1eac12
Compare


Description
This PR is for updating the PGA report to function as the 12-Month Donation Summary Report. The full scope of work is available here.
I'm doing most of the work in this PR but splitting out the print functionality into a separate one as that will introduce a good amount of complexity.
Here are the following tickets that this PR addresses:
Checklist: