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

fix: APP-531 create batch form button label missing and other bugs #2583

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

blushi
Copy link
Member

@blushi blushi commented Jan 21, 2025

Description

https://regennetwork.atlassian.net/browse/APP-531

While working on this, I also noticed a number of annoying bugs that I've fixed:

  1. when going through first step then saving and closing, then reloading and coming back to the first step, there was an error "Cannot read properties of undefined (reading 'toString')"
  2. infinite looping on CreditsBasic

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

From https://deploy-preview-2583--regen-marketplace.netlify.app/

  1. Log in with a wallet that is a class issuer and create a new credit batch from /dashboard/credit-batches
  2. Verify the "next" button is there
  3. Save step 1 data then reload the page and come back to step 1, no error should happen

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for terrasos ready!

Name Link
🔨 Latest commit aaef0a1
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/67923f57b1ad84000882fed3
😎 Deploy Preview https://deploy-preview-2583--terrasos.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

item => item.value.toString() === projectId.toString(),
);
if (isFound) saveProjectOptionSelected(isFound);
}, [projectId, projectOptions, projects, saveProjectOptionSelected]);
// adding projectOptions to dep array would cause infinite re-renders because it's an array,
Copy link
Member Author

Choose a reason for hiding this comment

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

fixing bug 2. (see PR description)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe another option could be to memoize projectOptions here using useMemo:

const memoizedProjectOptions = useMemo(() => projectOptions, [projectOptions]);

useEffect(() => {
  // ...
}, [projectId, saveProjectOptionSelected, memoizedProjectOptions]);

...(project as ProjectInfo),
metadata: projectsMetadata[i],
}));
return projectsLoading
Copy link
Member Author

Choose a reason for hiding this comment

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

fixing bug 1.

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit aaef0a1
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/67923f579bc82000080e37f1
😎 Deploy Preview https://deploy-preview-2583--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@blushi blushi changed the title fix: APP-531 create batch form button and other bugs fix: APP-531 create batch form button label missing and other bugs Jan 21, 2025
@blushi blushi requested a review from r41ph January 21, 2025 14:05
@blushi
Copy link
Member Author

blushi commented Jan 21, 2025

@erikalogie @S4mmyb see testing instructions

@erikalogie
Copy link
Collaborator

@blushi this seems to be working well. After I reloaded on step 2 of the flow and went back to step 1, the metadata I had input disappeared. Is that expected?

@blushi
Copy link
Member Author

blushi commented Jan 21, 2025

@blushi this seems to be working well. After I reloaded on step 2 of the flow and went back to step 1, the metadata I had input disappeared. Is that expected?

looks like yet another bug, I can have a quick look, else file a separate bug

item => item.value.toString() === projectId.toString(),
);
if (isFound) saveProjectOptionSelected(isFound);
}, [projectId, projectOptions, projects, saveProjectOptionSelected]);
// adding projectOptions to dep array would cause infinite re-renders because it's an array,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe another option could be to memoize projectOptions here using useMemo:

const memoizedProjectOptions = useMemo(() => projectOptions, [projectOptions]);

useEffect(() => {
  // ...
}, [projectId, saveProjectOptionSelected, memoizedProjectOptions]);

@blushi
Copy link
Member Author

blushi commented Jan 22, 2025

@r41ph re: #2583 (comment) I've tried that but for some reason, this doesn't help

@blushi
Copy link
Member Author

blushi commented Jan 22, 2025

@blushi this seems to be working well. After I reloaded on step 2 of the flow and went back to step 1, the metadata I had input disappeared. Is that expected?

looks like yet another bug, I can have a quick look, else file a separate bug

@erikalogie Fixed it here

@erikalogie
Copy link
Collaborator

LGTM

1 similar comment
@S4mmyb
Copy link
Member

S4mmyb commented Jan 23, 2025

LGTM

@blushi blushi force-pushed the fix-APP-531-create-batch-form-button branch from 1a8d9b7 to aaef0a1 Compare January 23, 2025 13:08
@blushi blushi merged commit f1394ca into dev Jan 23, 2025
18 checks passed
@blushi blushi deleted the fix-APP-531-create-batch-form-button branch January 23, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants