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

style(frontend): activity incomplete transaction list #3700

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

Conversation

BonomoAlessandro
Copy link
Collaborator

@BonomoAlessandro BonomoAlessandro commented Nov 20, 2024

Motivation

On the activity page we want to show a disclaimer to the user if he has active tokens that do not have an index canister yet. The disclaimer should list all affected tokens.

Changes

  • displays disclaimer

Tests

before:
image

after:

oisy-wallet-incomplete-transaction-list-new.mp4

@BonomoAlessandro BonomoAlessandro marked this pull request as ready for review November 20, 2024 14:29
@BonomoAlessandro BonomoAlessandro requested a review from a team as a code owner November 20, 2024 14:29
Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

Should we add a test as well?

</script>

<div class="flex flex-col gap-5">
<PageTitle>{$i18n.activity.text.title}</PageTitle>

{#if tokenList}
Copy link
Member

Choose a reason for hiding this comment

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

tokenList is never undefined, what's the goal of this check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check will return false if the tokenList is undefined, null or empty. If no token were found than the tokenList will be empty and the MessageBox will not be displayed. It works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

or empty.

I don't think so.

Capture d’écran 2024-11-22 à 09 07 51

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterpeterparker
But your test is wrong. The tokenList is not an array but the list of the tokens as string. It is an empty string.

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh my bad! Sorry!

Nitpick then: can you use notEmptyString(tokensList)

/**
* All user-enabled tokens matching the selected network or chain fusion that do not have an index canister.
*/
export const enabledNetworkTokensWithoutIndexCanister: Readable<Token[]> = derived(
Copy link
Member

Choose a reason for hiding this comment

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

  • Is there plan to reuse this information anywhere else? If not, I would scope the information to the component. I think a lean approach about adding new stores is for the best given our performance issues.

  • Why considering ETH and BTC? Should you just iterate on enabledIcrcTokens or enabledIcrcTokensNoCk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterpeterparker
No, at the moment it is not planed to reuse it anywhere else. I guess you are right, i should move the implementation into the component.

Why not? Isn't it possible that an ETH or BTC does not have an Index canister?

Copy link
Member

Choose a reason for hiding this comment

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

No, at the moment it is not planed to reuse it anywhere else. I guess you are right, i should move the implementation into the component.

I would say let's do that if that works for you?

Isn't it possible that an ETH or BTC

Canisters only existing on the Internet Computer that's why I'm wondering why we are considering Ethereum and Bitcoin, unless e.g. ckBTC or ckETH are listed explicitely within this derived stores but, I don't think it's the case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterpeterparker
That's ok for me, i just moved the implementation into the component.

Hmm, i guess you are right.

src/frontend/src/lib/i18n/en.json Outdated Show resolved Hide resolved
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