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

feat: separate approve and perform buttons #257

Merged
merged 3 commits into from Oct 11, 2022

Conversation

tnrdd
Copy link
Collaborator

@tnrdd tnrdd commented Oct 10, 2022

Description

Separate the approve and perform buttons to two different components.

Issue

fixes #252

Checklist:

  • My commit message follows the Conventional Commits specification
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my code
  • My changes generate no new warnings
  • My PR is rebased off the most recent develop branch
  • My PR is opened against the develop branch

Alert Reviewers

@codynhat @gravenp

@gravenp
Copy link
Member

gravenp commented Oct 10, 2022

Thanks for taking this one on quickly @tnrdd!

Just a couple issues/changes:

  • Can you change the "Reject Bid" button to red on the transaction details (2nd screen) so it matches the color on the parcel details page (1st screen). This was an oversight on my part.
  • When I edit a parcel's For Sale Price for the first time after claiming, I need to authorize ETHx transfer (for the stream buffer), but the screen is showing that no authorization transactions are required. See screenshot.

Screen Shot 2022-10-10 at 8 54 53 AM

  • When on the Reject Bid transaction details screen, the authorization & Reject Bid are rapidly alternating between active & disabled. After a while, it was settling on the authorization button as disabled and the Reject Bid button as active. As with the item above, this is incorrect as the penalty payment requires authorization to send. See screenshot.

Screen Shot 2022-10-10 at 8 57 29 AM

  • When reclaiming a parcel (wallet was the previous licensor) or making a foreclosure claim (wallet is not the previous licensor), I'm getting an error message that reads "Error: sending a transaction requires a signer (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=contracts/5.7.0)". See the screenshots below. I've confirmed that this is happening on the current develop branch as well, so this issue wasn't introduced here. I can open a new ticket as needed, but does this appear to be an issue with the Cadastre not triggering the transaction properly or something that should be addressed in the contacts (cc @codynhat)?

Screen Shot 2022-10-10 at 9 26 29 AM

Screen Shot 2022-10-10 at 9 32 36 AM

@codynhat
Copy link
Member

When reclaiming a parcel (wallet was the previous licensor) or making a foreclosure claim (wallet is not the previous licensor), I'm getting an error message that reads "Error: sending a transaction requires a signer (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=contracts/5.7.0)". See the screenshots below. I've confirmed that this is happening on the current develop branch as well, so this issue wasn't introduced here. I can open a new ticket as needed, but does this appear to be an issue with the Cadastre not triggering the transaction properly or something that should be addressed in the contacts (cc @codynhat)?

This is probably a regression from when I was adjusting the signers to prevent a bunch of RPC calls. Let's create a new ticket and address separately.

@gravenp
Copy link
Member

gravenp commented Oct 10, 2022

Created a separate ticket for the last issue here: #258

@gravenp gravenp requested a review from codynhat October 10, 2022 17:51
Copy link
Member

@gravenp gravenp left a comment

Choose a reason for hiding this comment

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

The latest changes look good and address the open issues (except for reclaiming/foreclosure which will get handled separately). Nice work, @tnrdd !

@codynhat - ready for your review.

@tnrdd tnrdd changed the title Feat/separate buttons feat: separate approve and perform buttons Oct 10, 2022
@codynhat
Copy link
Member

@tnrdd Can you pull in the latest merged PR from develop? @gravenp Can you verify the signer error is fixed before we merge?

@gravenp
Copy link
Member

gravenp commented Oct 11, 2022

Signer changes are working as expected with the latest develop branch pulled in.

@codynhat, this is ready for you again.

@codynhat codynhat merged commit aed464f into Geo-Web-Project:develop Oct 11, 2022
@tnrdd tnrdd deleted the feat/separate-buttons branch October 12, 2022 12:29
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.

None yet

3 participants