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

MVP Opera support #5432

Merged
merged 7 commits into from
Nov 5, 2019
Merged

MVP Opera support #5432

merged 7 commits into from
Nov 5, 2019

Conversation

owocki
Copy link
Contributor

@owocki owocki commented Oct 30, 2019

Description

MVP of Opera ssupport - tested on Opera + Chrome

Refers/Fixes

#5317 (comment)

Testing

Tested on Opera + Chrome

If approahc makes sense I will do one last batch of testing and merge

@owocki
Copy link
Contributor Author

owocki commented Oct 30, 2019

this one is for you @yorkerhodes3 !

@owocki owocki mentioned this pull request Oct 30, 2019
Closed
@codecov
Copy link

codecov bot commented Oct 30, 2019 •
edited
Loading

Codecov Report

Merging #5432 into master will increase coverage by 0.31%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5432      +/-   ##
==========================================
+ Coverage   29.67%   29.99%   +0.31%     
==========================================
  Files         242      242              
  Lines       20601    20713     +112     
  Branches     2968     3028      +60     
==========================================
+ Hits         6114     6213      +99     
- Misses      14236    14248      +12     
- Partials      251      252       +1
Impacted Files Coverage Δ
app/app/context.py 48.14% <ø> (ø) ⬆️
app/dashboard/views.py 12.94% <0%> (ø) ⬆️
...rketing/management/commands/no_applicants_email.py 0% <0%> (ø) ⬆️
...eting/management/commands/assemble_leaderboards.py 79.17% <0%> (+4.07%) ⬆️

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 f914886...f631fb5. Read the comment docs.

thelostone-mc
thelostone-mc reviewed Oct 31, 2019
View reviewed changes
app/assets/v2/js/shared.js Outdated
'<button id="metamask_connect" onclick="approve_metamask()">Click here to connect to metamask</button>';
$('#current-network').text(gettext('Metamask Not Connected'));
$('#navbar-network-banner').html(info);
if (window.ethereum._metamask) {
Copy link
Member

Choose a reason for hiding this comment

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

can we make this into an else if in L107 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cf5de1e

thelostone-mc
thelostone-mc reviewed Oct 31, 2019
View reviewed changes
app/assets/v2/js/shared.js Outdated
'<button id="metamask_connect" onclick="approve_metamask()">Click here to connect to metamask</button>';
$('#current-network').text(gettext('Metamask Not Connected'));
$('#navbar-network-banner').html(info);
if (window.ethereum._metamask) {
Copy link
Member

Choose a reason for hiding this comment

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

can we make this into an else if in L749?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cf5de1e

@thelostone-mc
Copy link
Member

approach looks alright to me ! If we can get one more ronud of testing in + address those comments -> we can get this in this week ❤️

@owocki
Copy link
Contributor Author

owocki commented Nov 4, 2019

just regression tested kudos send/ grants creation/tip sending/bounty creation on metamask and all went fine.. can i have a review @danlipert @thelostone-mc @octavioamu

danlipert
danlipert approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

Nice! Very clean

@octavioamu
Copy link
Contributor

octavioamu commented Nov 5, 2019 •
edited
Loading

Im trying to send a tip but I got stuck here, Is something more I need to do?
image
image
when I played with this I remember the problem in that was

web3.eth.sendTransaction({
            to: destinationAccount,
            value: amountInDenom,

since from: is missing

@octavioamu
Copy link
Contributor

octavioamu commented Nov 5, 2019 •
edited
Loading

Yes I added the from and now is working I will commit @owocki

image

@owocki
Copy link
Contributor Author

owocki commented Nov 5, 2019

thanks @octavioamu - can u push it back to my branch??

@octavioamu
Copy link
Contributor

Just tested kudos and tips working perfectly. Going to test a little more, excited with this

octavioamu
octavioamu approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

<3

@octavioamu octavioamu merged commit 399ca6c into master Nov 5, 2019
@owocki
Copy link
Contributor Author

owocki commented Nov 6, 2019

@octavioamu thanks for f631fb5 ; im concerned that there might be other places in the codebase where we implicitly expect from: to exist .. thereby peoples first opera experiences with gitcoin may fail..

since i wasnt able to reproduce the issue.. im not sure how to test for that tho.. what opera version r u on?

@octavioamu
Copy link
Contributor

Im with the last, tested with the desktop browser. Yes I think we can add the other places in a progressive way.

@owocki owocki mentioned this pull request Nov 19, 2019
Closed
@thelostone-mc thelostone-mc deleted the kevin/opera branch June 27, 2020 00:49
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

@octavioamu octavioamu octavioamu approved these changes

@danlipert danlipert danlipert 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.
4 participants
@owocki @thelostone-mc @octavioamu @danlipert