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

fix(WalletView): Apply to my wallet doesn't save or apply the order #14018

Merged

Conversation

caybro
Copy link
Member

@caybro caybro commented Mar 18, 2024

What does the PR do

  • we always want to save the changes, it's the toasts that we don't want to see when looking at the Advanced tab

Fixes #14016

Affected areas

Settings/Wallet/Manage tokens

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it
Zaznam.obrazovky.z.2024-03-18.19-01-24.webm

@caybro caybro linked an issue Mar 18, 2024 that may be closed by this pull request
@caybro caybro changed the title fix(WalletView): apply to wallet doesn't apply the order fix(WalletView): Apply to my wallet doesn't save or apply the order Mar 18, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Mar 18, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ cfb8132 #1 2024-03-18 17:12:20 ~5 min tests/nim 📄log
✔️ cfb8132 #1 2024-03-18 17:14:47 ~8 min macos/aarch64 🍎dmg
✔️ cfb8132 #1 2024-03-18 17:17:02 ~10 min tests/ui 📄log
✔️ cfb8132 #1 2024-03-18 17:19:28 ~12 min macos/x86_64 🍎dmg
✔️ cfb8132 #1 2024-03-18 17:22:35 ~16 min linux/x86_64 📦tgz
✔️ cfb8132 #1 2024-03-18 17:39:37 ~33 min windows/x86_64 💿exe
✔️ 9260910 #2 2024-03-18 22:40:38 ~5 min macos/aarch64 🍎dmg
✔️ 9260910 #2 2024-03-18 22:41:27 ~5 min tests/nim 📄log
✔️ 9260910 #2 2024-03-18 22:45:05 ~9 min macos/x86_64 🍎dmg
✔️ 9260910 #2 2024-03-18 22:45:26 ~9 min tests/ui 📄log
✔️ 9260910 #2 2024-03-18 22:51:20 ~15 min linux/x86_64 📦tgz
✔️ 9260910 #2 2024-03-18 23:08:18 ~32 min windows/x86_64 💿exe

Copy link
Contributor

@noeliaSD noeliaSD left a comment

Choose a reason for hiding this comment

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

Tested! LGTM!

Just added a comment / question.. not related to this PR but something we probably need to discuss again.

@@ -71,9 +71,10 @@ SettingsContentBase {
manageTokensView.saveChanges()
}
onSaveChangesClicked: {
manageTokensView.saveChanges()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering which is the difference between onSaveForLaterClicked and onSaveChangesClicked in terms of "saving changes".. For me, the difference should be.. onSaveChangesClicked should directly apply the new settings and set as well the order to the custom one, otherwise, this save for later doesn't have any sense to me... I think this is a point for future discussion, probably discussion with design team.

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed this already several times 😁 The current behavior is correct; the "Save for later" option just saves the Custom order but doesn't set it for the main wallet view

- we _always_ want to save the changes, it's the toasts that we don't
want to see when looking at the Advanced tab

Fixes #14016
@caybro caybro force-pushed the 14016-token-management-apply-to-wallet-doesnt-apply-the-order branch from cfb8132 to 9260910 Compare March 18, 2024 22:35
@caybro caybro merged commit 4b36054 into master Mar 19, 2024
8 checks passed
@caybro caybro deleted the 14016-token-management-apply-to-wallet-doesnt-apply-the-order branch March 19, 2024 09:35
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.

[Token Management] Apply to my wallet doesn't save or apply the order
5 participants