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: Fix order options bugs #1669

Merged
merged 2 commits into from
Jun 23, 2022
Merged

ui: Fix order options bugs #1669

merged 2 commits into from
Jun 23, 2022

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Jun 18, 2022

This diff fixes multiple bugs related to the order options:

  • The order options weren't being passed to the client
  • An additional range selection UI element was being displayed in each of the order option boxes
  • If the acceleration form was displayed, the order options would no longer work

Closes #1562

@@ -1786,12 +1795,9 @@ export default class MarketsPage extends BasePage {
async submitOrder () {
const page = this.page
Doc.hide(page.orderErr, page.vErr)
const order = this.parseOrder()
const order = this.currentOrder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the reason the order options weren't being passed to the client.

@@ -1027,7 +1027,7 @@ export class AccelerateOrderForm {
this.currencyUnit = preAccelerate.suggestedRange.yUnit
page.accelerateAvgFeeRate.textContent = `${preAccelerate.swapRate} ${preAccelerate.suggestedRange.yUnit}`
page.accelerateCurrentFeeRate.textContent = `${preAccelerate.suggestedRate} ${preAccelerate.suggestedRange.yUnit}`
OrderUtil.setOptionTemplates(page)
OrderUtil.setRangeOptionTempalate(page.accelerationRangeOptTmpl)
Copy link
Member

Choose a reason for hiding this comment

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

If the accelerationRangeOptTmpl is different than the rangeOptTmpl, then please make a new class. Maybe it can inherit XYRangeHandler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't different. I've done some refactoring.. all the templates are now in a template in forms.tmpl, and included in markets.tmpl and order.tmpl. I think it's much cleaner now.

@@ -215,7 +215,7 @@ export default class MarketsPage extends BasePage {
const success = () => { /* do nothing */ }
// Do not call cleanTemplates before creating the AccelerateOrderForm
this.accelerateOrderForm = new AccelerateOrderForm(page.accelerateForm, success)
Doc.cleanTemplates(page.rangeOptTmpl)
Doc.cleanTemplates(page.accelerationRangeOptTmpl)
Copy link
Member

@buck54321 buck54321 Jun 18, 2022

Choose a reason for hiding this comment

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

You can do this with the other templates a few lines down.

Comment on lines 245 to 250
const setOrderOptionTemplates = () => {
OrderUtil.setRangeOptionTempalate(page.rangeOptTmpl)
OrderUtil.setBooleanOptionTemplate(page.booleanOptTmpl)
OrderUtil.setOrderOptionTempalate(page.orderOptTmpl)
}
setOrderOptionTemplates()
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to separate these if you're not changing them (which you shouldn't be). Just pull out the new template in setOptionTemplates.

This diff fixes multiple bugs related to the order options:
- The order options weren't being passed to the client
- An additional range selection UI element was being displayed
  in each of the order option boxes
- If the acceleration form was displayed, the order options would
  no longer work
@JoeGruffins
Copy link
Member

Difficult to explain, but the first time I slide the sliders they pop back to zero now.

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.

LGTM. Tests well.

@chappjc
Copy link
Member

chappjc commented Jun 22, 2022

Difficult to explain, but the first time I slide the sliders they pop back to zero now.

I see the same thing. It appears that the first slide interaction (or rather parent element) checks the radio box first instead of adjusting the slider position. Screen cap:

slider.mp4

@JoeGruffins
Copy link
Member

Sorry to hijack but what program do you use for screen video captures? @chappjc

@chappjc
Copy link
Member

chappjc commented Jun 23, 2022

Peek

@@ -70,8 +70,9 @@ export default class OrderPage extends BasePage {
this.refreshOnPopupClose = true
}
// Do not call cleanTemplates before creating the AccelerateOrderForm
OrderUtil.setOptionTemplates(page)
Copy link
Member

@buck54321 buck54321 Jun 23, 2022

Choose a reason for hiding this comment

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

The setOptionTemplates thing is kinda a hack anyway. We should really just pass the page.*Tmpl elements to the form constructors. But no reason to mix it up now.

@chappjc chappjc merged commit ab73e4d into decred:master Jun 23, 2022
@chappjc chappjc added this to the 0.5 milestone Aug 26, 2022
@martonp martonp deleted the fixOrderOptions branch January 20, 2024 13:16
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.

Order options set on the UI do not get passed to the wallet
4 participants