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.

Sign up for GitHub

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

Jump to bottom

site: Fix js check for base and quote wallet availability #2577

Merged
merged 5 commits into from
Nov 10, 2023
Merged

site: Fix js check for base and quote wallet availability #2577

merged 5 commits into from
Nov 10, 2023

Conversation

ukane-philemon
Copy link
Contributor

@ukane-philemon ukane-philemon commented Oct 24, 2023 •
edited
Loading

Closes #2574

This MR fixes a bug that was introduced in #2548.

Improved the "noWallet" message in 70b3ae2 as suggested by @martonp.

Screenshot 2023-10-26 at 12 57 07 PM Screenshot 2023-10-26 at 12 56 45 PM

@buck54321 buck54321 mentioned this pull request Oct 31, 2023
Merged
buck54321
buck54321 requested changes Oct 31, 2023
View reviewed changes
Copy link
Member

@buck54321 buck54321 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

After creating one of two wallets from the markets view, the message still says to create both.
image

client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/locales.ts Outdated Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor Author

After creating one of two wallets from the markets view, the message still says to create both

Fixed in 0790b57. I noticed after wallet creation the missing this.market wallet is not updated.

martonp
martonp approved these changes Nov 2, 2023
View reviewed changes
client/webserver/site/src/js/markets.ts Outdated
Comment on lines 1141 to 1143
if (!noWalletMsg) {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!noWalletMsg) {
return
}
else return

@buck54321
Copy link
Member

After creating one of two wallets from the markets view, the message still says to create both

Fixed in 0790b57. I noticed after wallet creation the missing this.market wallet is not updated.

We should never be pulling the wallet from this.market to begin with, really. In fact, we shouldn't be storing the quote and base SupportedAsset either, but that's outside the scope of this PR. Your fix is OK for now.

buck54321
buck54321 approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Needs a rebase but testing well.

@ukane-philemon
fix js check for base and quote wallet availability
e1e4063 
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@ukane-philemon
improve noWallet message and add to translation
d9d4b31 
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@ukane-philemon
buck review changes
b34ceb4 
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@ukane-philemon
set missing asset wallet for selected market after wallet creation
24ccd44 
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@ukane-philemon
simplify
8cb203b 
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@buck54321 buck54321 merged commit f9e2610 into decred:master Nov 10, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martonp martonp martonp approved these changes

@buck54321 buck54321 buck54321 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ui: wallet forms not showing correctly on markets view
3 participants
@ukane-philemon @buck54321 @martonp