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

feat(frontend): use only groups in TokensDisplayHandler #3742

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

Conversation

AntonioVentilii
Copy link
Collaborator

@AntonioVentilii AntonioVentilii commented Nov 25, 2024

Motivation

We want to use util created in PR #3387 to group the tokens. The main change is that instead of returning a mix group of tokens and token-groups , we return only token groups.

Then, we treat differently the groups with only one token, showing them as single-token and not groups.

Changes

  • Use function groupTokens in component TokensDisplayHandler, instead of groupTokensByTwin.
  • Change the name and typing of the return array.
  • Change component TokensList to show the group component for groups and a single card for the single token.
  • Remove deprecated functions and types.

Tests

Screen.Recording.2024-11-25.at.15.15.07.mov

@AntonioVentilii AntonioVentilii marked this pull request as ready for review November 25, 2024 16:07
@AntonioVentilii AntonioVentilii requested a review from a team as a code owner November 25, 2024 16:07

// We start it as undefined to avoid showing an empty list before the first update.
export let tokens: TokenUiOrGroupUi[] | undefined = undefined;
export let tokenGroups: TokenUiGroup[] | undefined = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

It would be kind of nice to keep a variant group or token WDYT? Feel like it's a bit more more maintainable. Or am I overthinking it?

I mean something like

export type TokenUiOrGroupUi = TokenUi | TokenUi[]

// or even

export type TokenUiOrGroupUi = {token: TokenUi} | {group: GroupUi }

You know what I mean?

Again maybe I'm overengeering...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, understood! However, there is no real application for now: the list passed to Token list is just TokenGroups

If we add it, should I decompose the groups?

For example groupTokens(sortedTokens).reduce<{token: TokenUi} | {group: GroupUi }> ( ... ) ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe no real application but, this PR would introduces logic such as {@const token = tokenGroup.tokens[0]} and I'm not sure I'm absolutely into it...

For example groupTokens(sortedTokens).reduce<{token: TokenUi} | {group: GroupUi }> ( ... ) ?

Yes I would say so, do you think it's overeenginered?

I'm afraid that we start iterating on this and there we start having such logic as tokens[0] all over the place.

Copy link
Collaborator Author

@AntonioVentilii AntonioVentilii Nov 26, 2024

Choose a reason for hiding this comment

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

nono, i agree that tokens[0] is not good; however I need to re-think better how to do it, since we would be doing 2 loops in this way, 2 reduces (one in the grouping util and one here). I need to think how to merge it

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