Skip to content
This repository has been archived by the owner on Sep 27, 2023. It is now read-only.

UI fixes #199

Merged
merged 17 commits into from Apr 14, 2021
Merged

UI fixes #199

merged 17 commits into from Apr 14, 2021

Conversation

paouvrard
Copy link
Member

  • Allow the user to remove an Eth -> NEAR transfer before the lock tx is created.
    There is no confusion because the transfer was never initiated on-chain so this is effectively deleting the transfer.
    It is safer because the user may have another pending transaction with that token for example on uniswap, and if the allowance is consumed, the user will be stuck before the lock step with no way to recover. The allows the user to delete the transfer and start it again.
    Note that the approve transaction is not lost as creating a new transfer will skip the approve step if allowance is enough

  • Add restore functionality from landing page
    image

@paouvrard paouvrard requested a review from chadoh as a code owner April 8, 2021 03:08
@paouvrard
Copy link
Member Author

paouvrard commented Apr 8, 2021

#198
#187
#186
#184
#203
#191
#166

@chadoh
Copy link
Contributor

chadoh commented Apr 12, 2021

the user may have another pending transaction with that token for example on uniswap, and if the allowance is consumed, the user will be stuck before the lock step

How can this be? In one case, permission is given to the Uniswap contract to spend X tokens. In the other, permission is given to Token Locker contract to spend Y tokens. Can someone only give permission to spend certain ERC20 tokens to one contract at a time?

Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

Mostly this looks good, but the double-banner is a bummer. I don't think it's a good way to communicate with people. I'd like to see screenshots of it here, too, since the style of it also changed (no longer centered).

src/html/alert-banner.html Outdated Show resolved Hide resolved
src/html/bridge-erc20-form.html Outdated Show resolved Hide resolved
src/html/bridge-erc20-form.html Outdated Show resolved Hide resolved
src/html/alert-banner.html Show resolved Hide resolved
src/html/landing.html Outdated Show resolved Hide resolved
@@ -619,12 +617,13 @@ <h3 class="transfer__amount">${window.utils.formatLargeNum(
if (!(window.ethInitialized && window.nearInitialized)) return;

const transfers = await window.transfers.get();
const params = Object.keys(window.urlParams.get());
const currentParams = Object.keys(window.urlParams.get());
const allParams = ['new', 'erc20', 'erc20n']
Copy link
Contributor

Choose a reason for hiding this comment

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

"If current params includes all params, hide recent transfers. Otherwise, show recent transfers."

These names seem really confusing. allParams especially. Maybe irrelevantParams instead? "If params are irrelevant, hide. Else, show."

Copy link
Member Author

Choose a reason for hiding this comment

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

allParams are the pages of the app : new, erc20 and erc20n, so we want to display transfers if not one of those pages is active. Maybe appUrlPaths instead then ?

@heycorwin heycorwin self-requested a review April 12, 2021 21:11
@paouvrard
Copy link
Member Author

the user may have another pending transaction with that token for example on uniswap, and if the allowance is consumed, the user will be stuck before the lock step

How can this be? In one case, permission is given to the Uniswap contract to spend X tokens. In the other, permission is given to Token Locker contract to spend Y tokens. Can someone only give permission to spend certain ERC20 tokens to one contract at a time?

haha yes you are correct, I don't know why I thought that since allowance would be given to a different contract

@paouvrard paouvrard requested a review from chadoh April 13, 2021 02:59
@heycorwin
Copy link

heycorwin commented Apr 13, 2021

Pushed some changes to this branch.

Re: Indicating that the app is in beta, I think in general people understand that when something is in beta, it is still a WIP and might have some rough edges. As long as we can clearly indicate that it is in beta, we don't need to explicitly explain this via a big banner. I've added a "Beta" tag that appears near the logo in all instances. Once we're comfortable with the app's stability and feature-completeness, we can remove it.

Screen Shot 2021-04-13 at 11 25 40 AM

@heycorwin
Copy link

heycorwin commented Apr 13, 2021

@paouvrard could you let me know what exactly broke the build here? Can't seem to identify what might have caused it...

@paouvrard
Copy link
Member Author

@corwinharrell the build fails due to the commit message format: https://www.conventionalcommits.org/en/v1.0.0/

@paouvrard paouvrard merged commit 80dd558 into master Apr 14, 2021
@chadoh
Copy link
Contributor

chadoh commented Apr 14, 2021

🎉 This PR is included in version 3.4.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@heycorwin
Copy link

@corwinharrell the build fails due to the commit message format: https://www.conventionalcommits.org/en/v1.0.0/

@paouvrard ahh makes sense then why it went over my head. Happy to adhere to a specific format if that is helpful 👍

@mikedotexe mikedotexe deleted the pa_ui_fixes branch April 15, 2021 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants