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.

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

7531/outstanding commitments/special needs giving #849

Merged
merged 2 commits into from
Jan 12, 2024
Merged

7531/outstanding commitments/special needs giving #849

merged 2 commits into from
Jan 12, 2024

Conversation

wjames111
Copy link
Contributor

@wjames111 wjames111 commented Jan 9, 2024 •
edited
Loading

Description

  • Component rewrite required for Outstanding Recurring Commitments table
  • Component rewrite required for Outstanding Special Needs table
  • Ticket request available here

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

@wjames111
Copy link
Contributor Author

Created new clean branch from 7531-outstanding-commitments/special-needs-giving

@wjames111 wjames111 force-pushed the 7531/outstanding-commitments/special-needs-giving branch from d9535c1 to 7d8de96 Compare January 9, 2024 17:47
@aws-amplify-us-east-1 AWS Amplify (us-east-1)
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-849.d3dytjb8adxkk5.amplifyapp.com

@wjames111 wjames111 force-pushed the 7531/outstanding-commitments/special-needs-giving branch from 7d8de96 to e04da6f Compare January 10, 2024 17:35
canac
canac requested changes Jan 10, 2024
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 think the tests will fail until you make those two tweaks.

src/components/Coaching/CoachingDetail/OutstandingCommitments/OutstandingCommitments.test.tsx Outdated Show resolved Hide resolved
src/components/Coaching/CoachingDetail/OutstandingNeeds/OutstandingNeeds.test.tsx Outdated Show resolved Hide resolved
@canac canac added the On Staging Will be merged to the staging branch by Github Actions label Jan 10, 2024
@canac
Copy link
Contributor

canac commented Jan 10, 2024

Previous review history is in #834.

@wjames111 wjames111 force-pushed the 7531/outstanding-commitments/special-needs-giving branch 2 times, most recently from 2447464 to ad7bfcc Compare January 10, 2024 18:38
@wjames111 wjames111 force-pushed the 7531/outstanding-commitments/special-needs-giving branch from 1e84519 to de1a2cc Compare January 10, 2024 19:04
canac
canac reviewed Jan 12, 2024
View reviewed changes
src/components/Coaching/CoachingDetail/OutstandingCommitments/OutstandingCommitments.tsx Outdated
return theme.palette.statusDanger.main;
} else if (dateString.includes('month')) {
return theme.palette.statusWarning.main;
const checkDueDate = (expectedDate: string | null | undefined): string[] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great work! The only thing I'd change is having this return an object with a color property and an overdue property. Then when it is being used it will be a lot harder to accidentally mix them up.

FYI, the type you would want here is [string, string] because it is an array with exactly two elements. string[] is an array with an unknown number of elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, this function should be updated to return an object now. And good to know the type, I wasn't entirely sure on that one.

@wjames111 wjames111 force-pushed the 7531/outstanding-commitments/special-needs-giving branch from e189e84 to d426c49 Compare January 12, 2024 18:01
@wjames111 wjames111 force-pushed the 7531/outstanding-commitments/special-needs-giving branch from d426c49 to e8669cb Compare January 12, 2024 19:37
canac
canac approved these changes Jan 12, 2024
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.

🎉🎉🎉

@wjames111
Copy link
Contributor Author

🎉🎉🎉

I thought this day would never come hah.

@wjames111 wjames111 merged commit 98801d1 into main Jan 12, 2024
15 checks passed
@wjames111 wjames111 deleted the 7531/outstanding-commitments/special-needs-giving branch January 12, 2024 20:00
@dr-bizz
Copy link
Contributor

dr-bizz commented Jan 12, 2024

🎉🎉🎉

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

@canac canac canac approved these changes

Assignees
No one assigned
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wjames111 @canac @dr-bizz