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-7531 outstanding commitments/special needs giving #834

Closed

Conversation

wjames111
Copy link
Contributor

@wjames111 wjames111 commented Dec 5, 2023

Description

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

Potential Changes

  • Do we want pagination on either of these components? Maybe needed more for Outstanding Recurring Commitments table, as it generally shows a longer list of entries?
  • Possibly update component tests to use mock and generated values rather then a hardcoded set of data.
  • The overdue row could be included under the Expected Date row as shown in the original table?
  • As the two components push the page well past the side menu, should the components on this page be wrapped with overflow: scroll, or have the sidebar be sticky?

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

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

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

@wjames111
Copy link
Contributor Author

wjames111 commented Dec 5, 2023

@canac I'm thinking about combining OutstandingNeeds and OutstandingCommitments into one component. I'm not sure it's worth the extra time or not though. Do you think this is worth doing?

@canac
Copy link
Contributor

canac commented Dec 5, 2023

@wjames111 I think smaller components are more manageable to read, so I like how you have it right now with each table in its own component.

@wjames111
Copy link
Contributor Author

@canac Okay great, I'm going to do some touch ups, and run the yarn onesky:download script, then it should hopefully be ready for review. Sorry it's taken so long, I got hung up on the graphql query for a bit.

@wjames111
Copy link
Contributor Author

@canac It looks like these two additional components are causing CoachingDetail.test to fail. I can't seem to figure out what the issue is. I'm not all that comfortable with Jest yet, so any help you could give would be greatly appreciated.

@wjames111 wjames111 added the On Staging Will be merged to the staging branch by Github Actions label Dec 6, 2023
@wjames111 wjames111 marked this pull request as ready for review December 7, 2023 21:12
@wjames111 wjames111 requested a review from canac December 7, 2023 21:34
@wjames111 wjames111 changed the title 7531 outstanding commitments/special needs giving MPDX-7531 outstanding commitments/special needs giving Dec 7, 2023
@wjames111
Copy link
Contributor Author

This branch has been manually merged with staging to fix merge conflicts.

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.

Great work on this! 👍

@wjames111
Copy link
Contributor Author

@canac Thanks, couldn't have done without you. Appreciate the review, I'll get this updated hopefully by tomorrow depending on if I run into anymore issues.

@canac
Copy link
Contributor

canac commented Dec 7, 2023

  • Do we want pagination on either of these components? Maybe needed more for Outstanding Recurring Commitments table, as it generally shows a longer list of entries?

Yes, that would be good. The readme explains how to do it, and you'll want to use the useFetchAllPages variant.

  • Possibly update component tests to use mock and generated values rather then a hardcoded set of data.

Since your tests explicitly check the page for the values you passed in, it's probably OK.

  • The overdue row could be included under the Expected Date row as shown in the original table?

I'm not sure, but I lean towards making it match the original table as much as possible.

  • As the two components push the page well past the side menu, should the components on this page be wrapped with overflow: scroll, or have the sidebar be sticky?

#826 will make the sidebar collapsible on smaller screens. Would this fix the issue you're referring to?

@canac
Copy link
Contributor

canac commented Dec 7, 2023

Sure thing! I'm going to add @dr-bizz too to see if he has any thoughts.

@canac canac requested a review from dr-bizz December 7, 2023 22:04
@wjames111
Copy link
Contributor Author

  • Do we want pagination on either of these components? Maybe needed more for Outstanding Recurring Commitments table, as it generally shows a longer list of entries?

Yes, that would be good. The readme explains how to do it, and you'll want to use the useFetchAllPages variant.

  • Possibly update component tests to use mock and generated values rather then a hardcoded set of data.

Since your tests explicitly check the page for the values you passed in, it's probably OK.

  • The overdue row could be included under the Expected Date row as shown in the original table?

I'm not sure, but I lean towards making it match the original table as much as possible.

  • As the two components push the page well past the side menu, should the components on this page be wrapped with overflow: scroll, or have the sidebar be sticky?

#826 will make the sidebar collapsible on smaller screens. Would this fix the issue you're referring to?

The sidebar actually seems fine now, it initially looked like it was cutoff after scrolling but now it shows the whole way down. I did attempt to add pagination to the OutstandingCommitments component in my last commit, but I must be doing something terribly wrong. If you happen to know what that is please feel free to share.

@wjames111 wjames111 requested a review from canac December 8, 2023 15:08
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.

Were you going to add pagination to the other table? This should be basically done after you do that!

@wjames111 wjames111 requested a review from canac December 11, 2023 22: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.

Great work, just a couple of tweaks left! It looks like all my comments on OutstandingCommitments apply to OutstandingNeeds as well.

@wjames111
Copy link
Contributor Author

@canac the latest issues on this PR have been amended. Thanks for all your help on this, let me know if you catch anything else.

@canac
Copy link
Contributor

canac commented Dec 12, 2023

I just realized that we should make the tests more robust. Checking that the table headers are in the document is great, but we should also check that they show the data. Where you have pledgeCurrency: 'USD', in the mock, add some values for the other fields, and then in the test check that the correctly-formatted data is in the document. Let me know if you need help.

</GqlMockedProvider>,
);

expect(await getAllByTestId('Line').length).toEqual(8);
Copy link
Contributor

Choose a reason for hiding this comment

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

getAllByTestId doesn't return a promise, so the await here doesn't do anything, and the it function doesn't need to be async.

</GqlMockedProvider>,
);

expect(await findByText('Name')).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we do .toBeInTheDocument() in tests.

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 redid the tests to use .toBeInTheDocument() and removed the unnecessary async await. Also added additional data to specifically test for. Let me know if it needs any changes, Jest is still pretty new to me.

@wjames111
Copy link
Contributor Author

I just realized that we should make the tests more robust. Checking that the table headers are in the document is great, but we should also check that they show the data. Where you have pledgeCurrency: 'USD', in the mock, add some values for the other fields, and then in the test check that the correctly-formatted data is in the document. Let me know if you need help.

Yeah the test is super minimal, pretty much just to have it in there. I wanted to add more values to test for but the mock data seems to generate its own. That's helpful to know how to add specific values to check for, I'll get those added in.

@wjames111
Copy link
Contributor Author

@canac how are we looking?

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.

Great work with making those tests more specific! I have just a few suggestions. Most of comments apply to both tests.


const accountListId = 'account-list-1';

describe('LoadSkeleton', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally the root describe is the name of the component.

Suggested change
describe('LoadSkeleton', () => {
describe('OutstandingCommitments', () => {

expect(getAllByTestId('Line').length).toEqual(8);
});

it('Renders outstanding recurring commitments correctly', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally the it names aren't capitalized so that when combined with all the parent describe names, it reads like a sentence (i.e. "OutstandingCommitments renders outstanding recurring commitments correctly").

Suggested change
it('Renders outstanding recurring commitments correctly', async () => {
it('renders outstanding recurring commitments correctly', async () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I didn't think to format it that way. Thanks for pointing that out.

>
<OutstandingCommitments
accountListId={accountListId}
accountListType={AccountListTypeEnum.Coaching}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's switch this to AccountListTypeEnum.Own since our mocks are for an own account list.

</GqlMockedProvider>,
);

expect(getAllByTestId('Line').length).toEqual(8);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(getAllByTestId('Line').length).toEqual(8);
expect(getAllByTestId('Line')).toHaveLength(8);

pledges: {
nodes: [
{
amount: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's give this amount a value and check for it in the document since the test below checks for the N/A case.

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 great! Good job with everything!

Before we merge lets squash the commit history down to two commits: one for the outstanding commitments and one for the outstanding needs. That way we have a cleaner history in git.

@wjames111 wjames111 closed this Jan 9, 2024
@wjames111 wjames111 force-pushed the 7531-outstanding-commitments/special-needs-giving branch from bfe6262 to f3e1a02 Compare January 9, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants