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.

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

WOC: Adds tooltip hover on top of user input field. #2334

Merged
merged 2 commits into from Oct 5, 2018

Conversation

@usmanmuhd
Copy link
Contributor

commented Oct 3, 2018

Adds tooltip hover on top of user input field in Advaced payout and basic payout.
Fixes #2215
screenshot from 2018-10-05 06-18-37

screenshot from 2018-10-05 06-18-42

whatsapp image 2018-10-04 at 22 01 09

whatsapp image 2018-10-04 at 22 01 09 1

@codecov

This comment has been minimized.

Copy link

commented Oct 3, 2018

Codecov Report

Merging #2334 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2334      +/-   ##
==========================================
+ Coverage   28.48%   28.48%   +<.01%     
==========================================
  Files         145      145              
  Lines       11745    11742       -3     
  Branches     1593     1593              
==========================================
  Hits         3345     3345              
+ Misses       8286     8283       -3     
  Partials      114      114
Impacted Files Coverage Δ
app/avatar/utils.py 18.28% <0%> (+0.3%) ⬆️

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 1378743...ca8ba66. Read the comment docs.


</div>
<div id="tooltiphov">
<span id="heading">Where is my Eth Going?</span><br>

This comment has been minimized.

Copy link
@mbeacom

mbeacom Oct 3, 2018

Contributor

Can you wrap all of the copy throughout this file in {% trans "copy" %}

So here: {% trans "Where is my Eth Going?" %}

This comment has been minimized.

Copy link
@usmanmuhd

usmanmuhd Oct 3, 2018

Author Contributor

Is it only the text or even tags like <strong> that has to be moved to {% trans "<>" %}?

This comment has been minimized.

Copy link
@thelostone-mc
</div>
</div>
</div>
</div>
</div>
</div>
<style>
#tooltip {

This comment has been minimized.

Copy link
@mbeacom

mbeacom Oct 3, 2018

Contributor

Can we move this CSS to either the top of the template or possibly a separate .css file for bounties specifically?

This comment has been minimized.

Copy link
@usmanmuhd

usmanmuhd Oct 3, 2018

Author Contributor

bounty.css exists but I can't find that it is included anywhere.

This comment has been minimized.

Copy link
@mbeacom

mbeacom Oct 3, 2018

Contributor

It's not included in this template, but the majority of our common CSS styles are included via: {% include 'shared/head.html' %} cc @thelostone-mc @SaptakS What path would you like taken here? Do you want the styles left inline or moved to another asset?

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 3, 2018

Member

we should move it to it's own file! we have more css to be added here anyways (for the autocomplete )

@@ -201,5 +241,14 @@ <h5>{% trans 'Payout Preview' %}</h5>
<script src="{% static "v2/js/secrets.min.js" %}"></script>
<script src="{% static "v2/js/ethereumjs-accounts.js" %}"></script>
<script src="{% static "onepager/js/send.js" %}"></script>

<script type="text/javascript">
$("#tooltip").hover(

This comment has been minimized.

Copy link
@mbeacom

mbeacom Oct 3, 2018

Contributor

Can this move be to the bulk_payout.js ?

@@ -57,6 +57,7 @@ <h3>{% trans "Basic Payout" %}</h3>
</div>
<div class="w-100 my-2">
<label for=bountyFulfillment>{% trans "Bounty Submission:" %}</label>
<label id="tooltip">Where is my Eth going? <i class='fa fa-info-circle'></i></label>

This comment has been minimized.

Copy link
@mbeacom

mbeacom Oct 3, 2018

Contributor

Can you wrap all of the copy throughout this file in {% trans "copy here" %}

This comment has been minimized.

Copy link
@usmanmuhd

usmanmuhd Oct 3, 2018

Author Contributor

Yeah okay.

@@ -103,6 +114,36 @@ <h3>{% trans "Basic Payout" %}</h3>
document.token_address = '{{bounty.token_address}}';
</script>

<style>
#tooltip {

This comment has been minimized.

Copy link
@mbeacom

mbeacom Oct 3, 2018

Contributor

Can we move this to the top or into another css file? cc @thelostone-mc @SaptakS

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 3, 2018

Member

I'd be okay with us moving it to a new file ! let's do that

@@ -115,4 +156,14 @@ <h3>{% trans "Basic Payout" %}</h3>
<script src="{% static "v2/js/tokens.js" %}"></script>
<script src="{% static "v2/js/pages/shared_bounty_mutation_estimate_gas.js" %}"></script>
<script src="{% static "v2/js/pages/process_bounty.js" %}"></script>
<script type="text/javascript">

This comment has been minimized.

Copy link
@mbeacom

mbeacom Oct 3, 2018

Contributor

Can we move this to the bulk_payout.js ?

This comment has been minimized.

Copy link
@usmanmuhd

usmanmuhd Oct 3, 2018

Author Contributor

bulk_payout.js is not included in process_bounty.html. Maybe move it to a completely separate file?

@mbeacom mbeacom changed the title Adds tooltip hover on top of user input field. WOC: Adds tooltip hover on top of user input field. Oct 3, 2018

@owocki

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

  1. the modal is showing on the basic payout, but its not actually true for basic payout. for basic payout… the funds are sent dirctly to the fulfiller
  2. for tips/advanced payout/kudos , that modal should show up.. but the copy is not correct; we never have access to the funds… can we change the copy to say “this secure proxy address will hold the funds until the recipient claims them. this address is generated by gitcoin’s website, but we dont have your private keys, and the only person who can access the funds is the recipient” ?
@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

@usmanmuhd My man -> could have the changes in by tom ? would like to ship this asap ^_^

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

  1. the modal is showing on the basic payout, but its not actually true for basic payout. for basic payout… the funds are sent dirctly to the fulfiller
  2. for tips/advanced payout/kudos , that modal should show up.. but the copy is not correct; we never have access to the funds… can we change the copy to say “this secure proxy address will hold the funds until the recipient claims them. this address is generated by gitcoin’s website, but we dont have your private keys, and the only person who can access the funds is the recipient” ?

@owocki I will change it to just verified users on the basic payout page.
I will also add the tooltip for the tips ?

@owocki

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@owocki I will change it to just verified users on the basic payout page.

'verified users' is not how Basic Payout works. Basic payout works with Standard Bounties

here is some copy you can use:"Standard Bounties When you pay via Standard Bounties, your tokens will be securely taken from the escrow StandardBounties.sol contract on the Ethereum mainnet, and transferred to the payee's wallet"

I will also add the tooltip for the tips ?

yes pls!

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:Tooltip branch from 4629ad9 to 523265c Oct 4, 2018

@@ -0,0 +1,8 @@
$("#tooltip").hover(

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Oct 4, 2018

Strings must use singlequote. (quotes)

@@ -0,0 +1,8 @@
$("#tooltip").hover(
function(){

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Oct 4, 2018

Missing space before opening brace. (space-before-blocks)

@@ -0,0 +1,8 @@
$("#tooltip").hover(
function(){
$("#tooltiphov").css("display","block");

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Oct 4, 2018

Strings must use singlequote. (quotes)
A space is required after ','. (comma-spacing)

$("#tooltiphov").css("display","block");
},
function() {
$("#tooltiphov").css("display","none");

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Oct 4, 2018

Strings must use singlequote. (quotes)
A space is required after ','. (comma-spacing)

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:Tooltip branch from 523265c to 6f8faa3 Oct 4, 2018

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

@mbeacom @thelostone-mc I have made the changes as requested.
@owocki I do not have a design in order to add the tooltip in the tips page. Could you please help with that?

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:Tooltip branch 2 times, most recently from e81c109 to b1e4908 Oct 4, 2018

<div id="tooltiphov">
<span id="heading">{% trans "Where is my Eth Going?" %}</span><br>
<span id="subheading">{% trans "Your funds are safe!" %}</span><br><br>

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

can we remove the extra line?

@@ -85,6 +87,12 @@ <h3>{% trans "Basic Payout" %}</h3>
</a>
</div>
</div>
<div id="tooltiphov">
<span id="heading">{% trans "Where is my Eth Going?" %}</span><br>
<span id="subheading">{% trans "Your funds are safe!" %}</span><br><br>

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

could we indent by 2 spaces ?

color: #4A90E2;
float: right;
}
#tooltiphov {

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

new empty line btwn style declarations

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

rename this to tooltip--hover and update references ?

z-index: 10;
display: none;
}
#heading {

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

new empty line btwn style declarations

font-size: 18px;
color: #00A55E;
}
#hovertext{

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

new empty line btwn style declarations & add space before bracket. #hovertext {

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:Tooltip branch from b1e4908 to 980785f Oct 4, 2018

<span id="hovertext"><strong>{% trans "Unverified Users" %}</strong> <em>{% trans "(no eth address associated)" %}</em><br>
{% trans "A secure proxy address will hold the funds until the recipient claims them. This address is generated by gitcoin’s website, but we don't have its private keys, and the only person who can access the funds is the recipient." %}<br><br>

<strong>{% trans "Verified Users" %}</strong><br>

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

Stick to two space indentation

@@ -115,4 +122,5 @@ <h3>{% trans "Basic Payout" %}</h3>
<script src="{% static "v2/js/tokens.js" %}"></script>
<script src="{% static "v2/js/pages/shared_bounty_mutation_estimate_gas.js" %}"></script>
<script src="{% static "v2/js/pages/process_bounty.js" %}"></script>
<script src="{% static "v2/js/tooltiphov.js" %}"></script>

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

could you rename it to bounty/tooltip_hover.js and update the file name ?

@@ -201,5 +221,5 @@ <h5>{% trans 'Payout Preview' %}</h5>
<script src="{% static "v2/js/secrets.min.js" %}"></script>
<script src="{% static "v2/js/ethereumjs-accounts.js" %}"></script>
<script src="{% static "onepager/js/send.js" %}"></script>

<script src="{% static "v2/js/tooltiphov.js" %}"></script>

This comment has been minimized.

Copy link
@thelostone-mc
@@ -20,6 +20,14 @@
<head>
{% include 'shared/head.html' %}
{% include 'shared/cards.html' %}
<link rel="stylesheet" type="text/css" href="{% static "v2/css/tooltiphov.css" %}">

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Oct 4, 2018

Member

could you rename it to bounty/tooltip_hover.css and update the file name ?

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@usmanmuhd Left a few comments! Once that's done -> could you post a screenshot of it looks on mobile too ?

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:Tooltip branch 3 times, most recently from dd303e7 to d4f41e1 Oct 4, 2018

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

@thelostone-mc I have made the necessary changes. As for mobile, I am afraid hover does not work.

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:Tooltip branch from d4f41e1 to bd3128a Oct 4, 2018

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

@thelostone-mc Added the screenshots for mobile as well.

@thelostone-mc
Copy link
Member

left a comment

@usmanmuhd updated CSS a tad bit 👍 Thanks for turning this around quick :)

@owocki / @PixelantDesign we are good for payout.

x

addressed

@thelostone-mc thelostone-mc merged commit d1a2ad5 into gitcoinco:master Oct 5, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
stickler-ci No lint errors found.

@usmanmuhd usmanmuhd deleted the usmanmuhd:Tooltip branch Oct 5, 2018

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Shall I submit this on gitcoin?

@owocki

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

this is great! can we get it added on the tips page too? #2366

@@ -20,6 +20,33 @@
<head>
{% include 'shared/head.html' %}
{% include 'shared/cards.html' %}
<link rel="stylesheet" type="text/css" href="{% static "v2/css/tooltip_hover.css" %}">
<style type="text/css">

This comment has been minimized.

Copy link
@owocki

owocki Oct 5, 2018

Member

is there a reason this css is inline instead of in tooltip_hover.css?

This comment has been minimized.

Copy link
@usmanmuhd

usmanmuhd Oct 6, 2018

Author Contributor

It is basically reusing the #tooltip--hover design primarily designed keeping process_bounty.html in mind, and making a few changes to it.

@frankchen07 frankchen07 added this to In progress in Community Bounties via automation Nov 13, 2018

@frankchen07 frankchen07 moved this from In progress to Done in Community Bounties Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.