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(frontend): info box for ck conversions in Ethereum tokens #2116

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

feat(frontend): info box for ck conversions in Ethereum tokens #2116

wants to merge 6 commits into from

Conversation

AntonioVentilii-DFINITY
Copy link
Contributor

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY commented Aug 25, 2024 •
edited
Loading

Motivation

The info box about ck conversions will be shown in the twin tokens of ETH/ERC20 tokens too.

Changes

  • Create new Info component for Ethereum (similar to the one for IC)
  • Include component into Transactions page.

Tests

Screenshot 2024-08-25 at 14 55 56 Screenshot 2024-08-25 at 14 56 24

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY marked this pull request as ready for review August 27, 2024 10:53
@AntonioVentilii-DFINITY AntonioVentilii-DFINITY requested a review from a team as a code owner August 27, 2024 10:53
peterpeterparker
peterpeterparker reviewed Aug 27, 2024
View reviewed changes
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.

Wait, do I understand correctly that you want to display the InfoBox currently shown on ckETH and ckERC20 on ETH and ERC20?

If so, aren't we about to mix ICP and ETH-related components, since the InfoBox includes a call to action that opens a modal explaining how to convert the selected Ethereum token?

And if we're mixing those components, are you sure that all the necessary data will be loaded when displayed on Ethereum as well?

Have you tested all these cases and business flows?

Or am I misunderstanding something?

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY marked this pull request as draft August 27, 2024 12:32
@AntonioVentilii-DFINITY
Copy link
Contributor Author

Wait, do I understand correctly that you want to display the InfoBox currently shown on ckETH and ckERC20 on ETH and ERC20?

Correct! However this is still a draft, I made a mistake moving it to review, sorry. It still depends on other PRs.

@peterpeterparker
Copy link
Member

Thanks. As discussed with @anedos-dfinity, let's postpone displaying those boxes on ETH/ERC20 and implementing everything needed for their related conversion flows for a few weeks.

@peterpeterparker peterpeterparker mentioned this pull request Aug 28, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterpeterparker peterpeterparker peterpeterparker left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@AntonioVentilii-DFINITY @peterpeterparker @AntonioVentilii