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

feat: updated assets page for the EVM account #900

Merged
merged 8 commits into from
Aug 25, 2023
Merged

feat: updated assets page for the EVM account #900

merged 8 commits into from
Aug 25, 2023

Conversation

impelcrypto
Copy link
Member

@impelcrypto impelcrypto commented Aug 15, 2023 •
edited

Pull Request Summary

  • feat: updated assets page for the EVM account

Check list

  • contains breaking changes
  • adds new feature
  • modifies existing feature (bug fix or improvements)
  • relies on other tasks
  • documentation changes
  • tested on mobile devices

This pull request makes the following changes:

Changes

  • feat: updated assets page for the EVM account
    • combined both XC20 and ERC20 assets on the same panel
    • put native token on the top
  • fix: hid XVM assets panel because AA might replace it

=Before=
image

=After=

image

Search USD
image

@github-actions
Copy link

github-actions bot commented Aug 15, 2023 •
edited

Visit the preview URL for this PR (updated for commit 18fbd79):

https://astar-apps--pr900-feat-update-evm-asse-1mgc47w9.web.app

(expires Fri, 01 Sep 2023 06:38:04 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: dd76fe72958fe2910fef9d53f0b4539b82b849db

@impelcrypto impelcrypto changed the title feat: updated assets page for EVM accounts feat: updated assets page for the EVM account Aug 18, 2023
@impelcrypto impelcrypto marked this pull request as ready for review August 22, 2023 04:58
gluneau
gluneau approved these changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@gluneau gluneau left a comment

Choose a reason for hiding this comment

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

looks good, new tests passes locally

@Kahonnohak
Copy link
Contributor

Looks great!!

Would you like to merge it? or wait until Unified account?

If you are merging please add the same border line with what we have in Astar Native.

Screenshot 2023-08-24 at 13 09 25

@impelcrypto
Copy link
Member Author

@Kahonnohak

Would you like to merge it? or wait until Unified account?

I'll merge it today.

If you are merging please add the same border line with what we have in Astar Native.

Updated.
image

bobo-k2
bobo-k2 approved these changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@bobo-k2 bobo-k2 left a comment

Choose a reason for hiding this comment

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

LGTM

src/components/assets/EvmNativeToken.vue
getTokenImage({ isNativeToken: true, symbol: nativeTokenSymbol.value })
);

const updateStates = async (nativeTokenUsd: number): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should not have business or data logic in components. You don't need to do refactor now in this PR, but have in mind for the future components. I will try to remind myself also

Kahonnohak
Kahonnohak approved these changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@Kahonnohak Kahonnohak left a comment

Choose a reason for hiding this comment

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

LGTM

@impelcrypto impelcrypto merged commit 0294f2e into main Aug 25, 2023
6 checks passed
@impelcrypto impelcrypto deleted the feat/update-evm-assets branch August 25, 2023 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gluneau gluneau gluneau approved these changes

@bobo-k2 bobo-k2 bobo-k2 approved these changes

@Kahonnohak Kahonnohak Kahonnohak approved these changes

@ayumitk ayumitk Awaiting requested review from ayumitk

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@impelcrypto @Kahonnohak @gluneau @bobo-k2