-
Notifications
You must be signed in to change notification settings - Fork 92
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: Hide old errors on market page forms #1662
Conversation
Doc.hide(page.vErr) | ||
this.showForm(page.verifyForm) |
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.
Strikes me as a bit backwards to clean up when opening the form instead of when closing the form.
Would the following not be targeted well enough?
diff --git a/client/webserver/site/src/js/markets.ts b/client/webserver/site/src/js/markets.ts
index f7a791e0..c4a1ff03 100644
--- a/client/webserver/site/src/js/markets.ts
+++ b/client/webserver/site/src/js/markets.ts
@@ -381,8 +381,9 @@ export default class MarketsPage extends BasePage {
setOrdersSortColClasses()
const closePopups = () => {
- Doc.hide(page.forms)
+ Doc.hide(page.forms, page.vErr)
page.vPass.value = ''
+ page.vErr.textContent = ''
page.cancelPass.value = ''
}
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.
Both of these work, but I was thinking of it more of setting up the form to be displayed instead of cleaning it up. I actually see now that all of the forms on this page have the same issue, so I'll update those as well.
For a lot of the forms we have a refresh
method that sets up the form. How about we make it the standard from now on to always have a refresh function for every popup form which must be called whenever you open it? Then, when forms are reused, these cleanup things don't have to be repeated in the closePopups
function of every page. These closePopups
methods can get very big when you need to handle the cleanup for each popup on the page each time you close one of them.
Some of the forms on the markets page would show an old error when opening the form. This is now fixed.
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.
LGTM
Some of the forms on the markets page would show an old error when opening the form. This is now fixed.