Skip to content

MPDX-8850 Connect GraphQL to Ministry Reminders #1548

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.

Sign up for GitHub

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

Jump to bottom
Open
kegrimes wants to merge 9 commits into
base: main
Choose a base branch
from
Open

MPDX-8850 Connect GraphQL to Ministry Reminders #1548

kegrimes wants to merge 9 commits into from
+464 −330

Conversation

@kegrimes
Copy link
Contributor

@kegrimes kegrimes commented Dec 13, 2025 •
edited
Loading

Description

Connect graphQL query and update mutation to Ministry Partner Reminders frontend.

Testing

  • Go to /6bbc265f-ec81-4b23-b297-5bd298c4d619/reports/mpReminders
  • Scroll through the donor list
  • Check that infinity scroll still works
  • Change a select option & refresh the page
  • Check that it successfully updated

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 "Preview" to automatically create a preview environment)
  • I have run the Claude Code /pr-review command locally and fixed any relevant suggestions
  • I have requested a review from another person on the project
  • I have tested my changes in preview or in staging
  • I have cleaned up my commit history

@kegrimes kegrimes added the Staging API Run GraphQL codegen against the staging API label Dec 13, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 13, 2025 •
edited
Loading

Bundle sizes [mpdx-react]

Compared against 92a3ae1

No significant changes found

@kegrimes kegrimes requested a review from wjames111 December 29, 2025 20:11
wjames111
wjames111 reviewed Dec 29, 2025
View reviewed changes
Copy link
Contributor

@wjames111 wjames111 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm dumb I thought it autosaved, looks really good!

src/components/Reports/MinistryPartnerReminders/MPRemindersReport.tsx
<SimpleScreenOnly>
<Box mt={3} mb={4}>
<Trans i18nKey={'reminders.description'}>
<p style={{ lineHeight: 1.5 }}>
Copy link
Contributor

@wjames111 wjames111 Dec 29, 2025

Choose a reason for hiding this comment

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

I'd stick with MUI wherever you can.

Copy link
Contributor Author

@kegrimes kegrimes Dec 30, 2025

Choose a reason for hiding this comment

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

Daniel suggested I use <Trans> for readability when there are longer sentences. What do you think?

Copy link
Contributor

@wjames111 wjames111 Dec 30, 2025

Choose a reason for hiding this comment

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

Oh sorry yeah Trans is great, I was talking about the paragraph tags.

@wjames111
Copy link
Contributor

wjames111 commented Dec 29, 2025

Do you think we could make this save on change rather then a specific save button, I was being dumb but I do think it's a little more intuitive that way or even moving the save button below the list rather then above it.

@kegrimes
Copy link
Contributor Author

kegrimes commented Dec 30, 2025 •
edited
Loading

I think the original design on staff web had the save button below the table. Daniel thought that it should go above so people don't have to scroll down to see that they need to save it. I think autosaving could be good, but also since the list of donors can be very large, maybe users want to double check what they are changing before implementing those changes. Or maybe they want to revert back to their original changes, but since it is a large list, they wouldn't be able to remember what was the previous status if we use autosave. What do you think? We could definitely get Daniel or Ryan's opinion on it.

Also, currently I am just picking the first designation account that is returned. I don't think that should be the case, but I am also not completely sure how to differentiate which account is desired. Would you happen to have any ideas?

@kegrimes kegrimes requested a review from wjames111 December 30, 2025 13:32
@wjames111
Copy link
Contributor

wjames111 commented Dec 30, 2025

@kegrimes those are fair points, and probably a good reason to stick with the save button. If Daniel wanted it up top then go with that. You could always double check with Ryan, but it sounds like the way you have it is good.

So is the problem that there are many designation accounts and each designation account has many ministry partner reminders, but you just want to list all ministry partner reminders?

wjames111
wjames111 approved these changes Dec 30, 2025
View reviewed changes
Copy link
Contributor

@wjames111 wjames111 left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes! I think we just need to figure out which designation account to show now. If I understand the issue correctly, I don't have any good solutions at the moment. It might need to wait for Caleb or Daniel's input. Running it by Ryan might also be a good idea.

@kegrimes
Copy link
Contributor Author

kegrimes commented Dec 31, 2025

@dr-bizz @canac I am currently hardcoding the report to display the ministry partners for the first designation account returned by the query. I am wondering what is the intended logic for selecting which to display. I may be missing some context on how this is done. Let me know what your thoughts are!

canac
canac reviewed Jan 5, 2026
View reviewed changes
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 am currently hardcoding the report to display the ministry partners for the first designation account returned by the query. I am wondering what is the intended logic for selecting which to display. I may be missing some context on how this is done. Let me know what your thoughts are!

A lot of the reports have a designation account selector in the sidebar. Maybe you could do something similar? I'm wondering though why the GraphQL endpoint makes the designation account filter required? I wonder if it would be possible to show contacts from all designation accounts so that users don't have to select one unless they truly need to filter by designation account. That would be a question for whoever set up the GraphQL endpoint.

src/components/Reports/MinistryPartnerReminders/Table/PrintTable.tsx
<Typography sx={{ fontSize: '14px' }}>{row.partner}</Typography>
<Typography sx={{ fontSize: '14px' }}>
{row.partnerId
? t('({{partnerId}})', { partnerId: row.partnerId })
Copy link
Contributor

@canac canac Jan 5, 2026

Choose a reason for hiding this comment

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

Since there aren't any letters in this label, I don't think we need to localize it.

Suggested change
? t('({{partnerId}})', { partnerId: row.partnerId })
? `(${row.partnerId})`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@canac canac canac left review comments

@wjames111 wjames111 wjames111 approved these changes

Assignees

No one assigned

Labels

Staging API Run GraphQL codegen against the staging API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kegrimes @wjames111 @canac