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

ui: Add table showing available markets when registering for dex. #1135

Merged
merged 3 commits into from Aug 26, 2021

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Jul 22, 2021

This PR adds a table on the confirm register form to display what markets the DEX provides.
Screen Shot 2021-07-21 at 3 29 28 PM

Also there is a bit of refactoring since the form was used in multiple places, but the code was completely replicated.

Closes #1125.

@martonp martonp changed the title Add table showing available markets when registering for dex. ui: Add table showing available markets when registering for dex. Jul 22, 2021
@chappjc
Copy link
Member

chappjc commented Jul 23, 2021

Looks very nice, @martonp!

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Looks good to me, see below simnet harness result:

image

Refactoring of forms and other frontend stuff looks good to me too, but @buck54321 will have to review all that since it's more extensive than I anticipated. The refactoring does look positive though with narrower concerns (e.g. getCertFile, getDexAddr, registerDEXFundsFail, etc), but it's not my strong suit.

@martonp
Copy link
Contributor Author

martonp commented Jul 31, 2021

@chappjc I updated the bind function to be a class. All the logic stayed the same though.

@chappjc chappjc added the UI label Aug 2, 2021
@chappjc chappjc added this to the 0.3 milestone Aug 3, 2021
@chappjc
Copy link
Member

chappjc commented Aug 5, 2021

A good number of conflicts created for you @martonp with the app seed PR merged, although this is just about ready to go pending a look from buck.

@martonp
Copy link
Contributor Author

martonp commented Aug 6, 2021

I think it's better to wait for #1119 to be merged first, since that will cause some more conflicts here.

@chappjc
Copy link
Member

chappjc commented Aug 19, 2021

This is now unblocked with #1119 merged.

@martonp
Copy link
Contributor Author

martonp commented Aug 23, 2021

This is merged now.

<span {{if $passwordIsCached}}class="d-hide"{{end}}>Enter your app password to confirm DEX registration.</span>
When you submit this form, <span id="feeDisplay"></span> DCR will be spent from your Decred wallet to pay
registration fees.
Enter your app password to confirm DEX registration.
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this line needs to be hidden conditionally too.

Also, apologies in advance because we're likely going to merge #1051 first since we keep making @buck54321 update it. When that goes in, you'll have to resolve the differences with the confirmRegistrationForm template (and any other conflicts), define any new strings in client/webserver/locales/en-us.go, then do the go generate command in client/webserver/site to rebuild the localized_html files. This is going to be part of the site workflow going forward, so we might as well trial it with you after #1051 goes in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good, no problem.

@chappjc
Copy link
Member

chappjc commented Aug 24, 2021

With PR #1051 merged, you'll need to update to with strings in client/webserver/locales/en-us.go and the new [[[ adf ]]] notation in the html templates, then rebuilid the localized_html with a go generate in client/webserver/site. The conflicts in confirmRegistrationForm are relatively minor though. Thanks, @martonp!

@martonp
Copy link
Contributor Author

martonp commented Aug 24, 2021

I'm currently hiding the password field if the user selected the Remember my password checkbox, but not using the password cache in the register workflow. I think it's better this way because the submit button on this form causes them to spend money. Let me know if you guys agree though.

@chappjc
Copy link
Member

chappjc commented Aug 24, 2021

Makes sense to me to always require pass confirmation for register for the reasons you stated, plus it's a relatively rare event.

@chappjc chappjc requested a review from buck54321 August 25, 2021 12:40
@chappjc

This comment has been minimized.

@chappjc
Copy link
Member

chappjc commented Aug 25, 2021

Geeze, nevermind that, it's alright just my tempates out of date.

Looks good on my simnet harness:

image

@@ -26,10 +24,10 @@ export default class RegistrationPage extends BasePage {
// Form 3: Unlock Decred wallet
'unlockWalletForm',
// Form 4: Configure DEX server
'dexAddrForm',
'dexAddrForm', 'dexAddr',
// Form 5: Confirm DEX registration and pay fee
'confirmRegForm', 'feeDisplay', 'dcrBaseMarketName', 'dexDCRLotSize', 'appPass', 'submitConfirm', 'regErr',
Copy link
Member

Choose a reason for hiding this comment

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

Can clean up some of these. dexDCRLotSize is gone from markets.tmpl now, maybe others.

Comment on lines 293 to 304
/**
* onCertFileChange when the input certFile changed, read the file
* and setting cert name into text of selectedCert to display on the view
*/
async onCertFileChange () {
const page = this.page
const files = page.certFile.files
if (!files.length) return
page.selectedCert.textContent = files[0].name
Doc.show(page.removeCert)
Doc.hide(page.addCert)
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe this method is unused.

return cert
}

/* gets the dex address inputted by the user */
Copy link
Member

Choose a reason for hiding this comment

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

inputted -> input

@chappjc chappjc merged commit 027b480 into decred:master Aug 26, 2021
@martonp martonp deleted the marketTable branch December 20, 2022 22:09
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.

client,ui: register dialog should summarize the DEX markets
4 participants