-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
Add cancel modal on cancelling grants #2995
Conversation
7c0fb29
to
7c800e5
Compare
Codecov Report
@@ Coverage Diff @@
## grantz #2995 +/- ##
=========================================
- Coverage 30.21% 30.2% -0.02%
=========================================
Files 190 190
Lines 14264 14225 -39
Branches 1881 1872 -9
=========================================
- Hits 4310 4296 -14
+ Misses 9815 9791 -24
+ Partials 139 138 -1
Continue to review full report at Codecov.
|
@SaptakS conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I left a comment.
@@ -70,6 +110,7 @@ $(document).ready(function() { | |||
.send({from: accounts[0], gasPrice: 4000000000}) | |||
.on('transactionHash', function() { | |||
document.issueURL = document.getElementById('grant-link').href; | |||
$('.modal .close').trigger('click'); | |||
enableWaitState('#grants-details'); | |||
}) | |||
.on('confirmation', function(confirmationNumber, receipt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im worry about how we are doing this if someone decide to reload the page in the waitState we have some kind of mechanist to check if the grants is sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this question is there in all the pages if someone refreshes at this stage. It will show the particular page instead of the waiting state. I think not related to this PR. But I will check on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes yes agree is not related at all but yesterday was looking the cancel code and started to think about the closed grants in blockchain but not in our db
3d9f115
to
11d3ce4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hold on merging this until #3012 gets merged? Where the modal is fired and how the form is being submitted will change with the above PR.
@captnseagraves sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally lgtm. we just need to resolve the conflict since the grant editing and cancellation changes went in.
11d3ce4
to
99c18db
Compare
@mbeacom @captnseagraves @thelostone-mc please have a look one last time. :) |
Description
Adds cancel modal based on the design by @willsputra
Checklist
Affected core subsystem(s)
Testing
Refers/Fixes