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

Disable import button on maximum #5557

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mstrasinskis
Copy link
Contributor

@mstrasinskis mstrasinskis commented Oct 1, 2024

Motivation

Due to backend validation, it is currently not possible to import more than 20 tokens. To improve the user experience, we disable the import button when the user has already imported the maximum number of custom tokens.

Changes

  • Display a disabled import button when maximum reached.
  • Don’t display the import button until the imported tokens have loaded, to ensure its correct status is shown.

Tests

  • Unit tests updated.
  • Manually:
Enabled Disabled
image image

Todos

  • Add entry to changelog (if necessary).
    Not necessary.

@mstrasinskis mstrasinskis changed the title Mstr/disable import button Disable import button on maximum Oct 1, 2024
@mstrasinskis mstrasinskis marked this pull request as ready for review October 1, 2024 09:37
@mstrasinskis mstrasinskis requested a review from a team as a code owner October 1, 2024 09:37
@@ -1061,6 +1061,7 @@
"link_to_dashboard": "https://dashboard.internetcomputer.org/canister/$canisterId",
"add_index_canister": "Add Index Canister",
"add_index_description": "Transaction history is not available. To see this token’s transaction history in the NNS dapp, you need to provide an index canister. <strong>Note:</strong> not all tokens have index canisters.",
"failed_tooltip": "The NNS dapp couldn’t load an imported token. Please try again later, or contact the developers."
"failed_tooltip": "The NNS dapp couldn’t load an imported token. Please try again later, or contact the developers.",
"maximum_reached_tooltip": "You can import maximum $max tokens."
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "You've already imported the maximum number of $max tokens. You need to remove another token before you can import more."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated to "You can import maximum $max tokens. You need to remove a token before you can import more."

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason I suggested a different message is because "You can import maximum $max tokens." is not grammatically correct English. If you want to keep it as close as possible to the original, it should at least be "You can import a maximum of $max tokens."
I suggested something longer because I thought it would be more helpful and natural. But mostly I don't want it to be broken English.

>
<IconPlus />{$i18n.import_token.import_token}
</button>
{#if maximumImportedTokensReached}
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need an if/else with the button element in both cases.
The Tooltip is automatically disabled if the text attribute is empty or absent.
So you can just only pass the text attribute if the maximum is reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, didn't know about the text attribute. Done.


setImportedTokens(MAX_IMPORTED_TOKENS - 1);
const po = renderPage([]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Add await runResolvedPromises(); here as well for consistency to make sure that's not the only reason why the expectations below are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not. Done.

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.

2 participants