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

Show Just 1 Alert Banner for ERC20 Token #1668

Merged
merged 1 commit into from
Aug 3, 2018
Merged

Show Just 1 Alert Banner for ERC20 Token #1668

merged 1 commit into from
Aug 3, 2018

Conversation

pinkiebell
Copy link
Contributor

Fixes #1566

@codecov
Copy link

codecov bot commented Jul 9, 2018 •
edited
Loading

Codecov Report

Merging #1668 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1668   +/-   ##
======================================
  Coverage    25.5%   25.5%           
======================================
  Files         130     130           
  Lines       10686   10686           
  Branches     1425    1425           
======================================
  Hits         2725    2725           
  Misses       7883    7883           
  Partials       78      78

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d55df90...1f87b78. Read the comment docs.

thelostone-mc
thelostone-mc reviewed Jul 16, 2018
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could we change the alert to a warning if it's an error?

app/assets/v2/js/shared.js Outdated
if (!document.alert_enable_token_shown) {
_alert("You have not yet enabled this token. To enable this token, go to the <a style='padding-left:5px;' href='/settings/tokens'> Token Settings page and enable it</a>. (this is only needed one time per token)");
}
document.alert_enable_token_shown = true;
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be a warning alert like here

@pinkiebell
Copy link
Contributor Author

Ahh, will rebase ;)

@thelostone-mc
Copy link
Member

thelostone-mc commented Jul 16, 2018 •
edited
Loading

@pinkiebell Oh I meant , just make it a warning alert by default but this could work too !
@mbeacom ?

Could you squash the commits into one neat commit ?
PS: Also what's the content of error.toString()
Need to make sure we aren't showing info that's not needed for the user

@pinkiebell
Copy link
Contributor Author

pinkiebell commented Jul 16, 2018 •
edited
Loading

@thelostone-mc
Hopefully a sophisticated error message. But I really don't know, the error object is always null no matter what I pass. However that might change in the future and could provide more info if something unexpected happens.

Let me know if you want me to revert back to just the warning message.

@pinkiebell
Copy link
Contributor Author

@thelostone-mc
https://github.com/ethereum/web3.js/blob/54e40bb4bbb97ba9590c3ba6f933951c7a04dd16/lib/web3/errors.js

@thelostone-mc
Copy link
Member

thelostone-mc commented Jul 16, 2018 •
edited
Loading

Let's just stick to warning for now and do away with displaying the error message 👍
That and squash the commits and we should be good for merge

@pinkiebell
Copy link
Contributor Author

@thelostone-mc
I pushed a little update, is this good to go?

thelostone-mc
thelostone-mc approved these changes Aug 3, 2018
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

SaptakS
SaptakS approved these changes Aug 3, 2018
View reviewed changes
Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

lgtm. Merging

@SaptakS SaptakS merged commit c51e204 into gitcoinco:master Aug 3, 2018
@pinkiebell pinkiebell deleted the 1566 branch November 6, 2018 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thelostone-mc thelostone-mc thelostone-mc approved these changes

@SaptakS SaptakS SaptakS approved these changes

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
@pinkiebell @thelostone-mc @SaptakS