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

Following section #6301

Merged
merged 18 commits into from
Apr 29, 2020
Merged

Following section #6301

merged 18 commits into from
Apr 29, 2020

Conversation

cupOJoseph
Copy link
Contributor

@cupOJoseph cupOJoseph commented Mar 26, 2020

Description

New tab on use profile for followers.

Refers/Fixes

#6095

Testing

Tested:
New Follow tab on profile, url addition for following and follower subtabs, inline follow buttons, grid of users, also following list, email and chat buttons, hidden email when None

@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #6301 into master will increase coverage by 0.01%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6301      +/-   ##
==========================================
+ Coverage   27.07%   27.08%   +0.01%     
==========================================
  Files         291      291              
  Lines       26682    26702      +20     
  Branches     3948     3951       +3     
==========================================
+ Hits         7223     7232       +9     
- Misses      19192    19203      +11     
  Partials      267      267              
Impacted Files Coverage Δ
app/dashboard/views.py 11.20% <0.00%> (-0.04%) ⬇️
app/dashboard/models.py 49.75% <44.44%> (-0.04%) ⬇️
app/retail/emails.py 22.34% <0.00%> (ø)
app/dashboard/sync/etc.py 14.63% <0.00%> (ø)
app/grants/management/commands/migrate_grant.py 0.00% <0.00%> (ø)
app/dashboard/embed.py 31.60% <0.00%> (+3.44%) ⬆️

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 570abd3...1dccfb9. Read the comment docs.

@octavioamu
Copy link
Contributor

Nice, Is this working? Asking because says "is not working yet"

@cupOJoseph
Copy link
Contributor Author

It will compile, but missing some of the features

I'm using mailto:{{user.profile.data.email}} for the email buttons in the list, but they are always returning the email as None. I need to fix that, working on it now.

Also, the following button color seems inconsistent in the design. asking Alisa now.

@octavioamu
Copy link
Contributor

cool, send some screenshots here or video so we can check. Thanks man!

@cupOJoseph
Copy link
Contributor Author

@octavioamu should be good to go now.
screen1

Copy link
Contributor Author

@cupOJoseph cupOJoseph left a comment

Choose a reason for hiding this comment

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

Which master did this pull from?, I dont see if it made any changes

@cupOJoseph
Copy link
Contributor Author

Just made an update to hide email button when no email is there, and fix blue follow button not working

@cupOJoseph
Copy link
Contributor Author

cupOJoseph commented Apr 6, 2020

@octavioamu can we have this reviewed when you get a chance? thanks

Screen Shot 2020-04-06 at 10 02 54 AM

Screen Shot 2020-04-06 at 10 03 08 AM

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.

@jschiarizzi we need to remove the email button - we can't be exposing user emails like this. If we want user-to-user messaging, we need to just use chat. CC: @PixelantDesign @owocki

@cupOJoseph
Copy link
Contributor Author

Okay, that was what was asked for. I thought that icon was already used in gitcoin profiles, but it turns out that is an invite link to this "https://gitcoin.co/users?invite=jschiarizzi"
Maybe we can replace it with that?

Copy link
Contributor Author

@cupOJoseph cupOJoseph left a comment

Choose a reason for hiding this comment

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

@danlipert here is the change you requested. No email, instead replaced with the invite to bounty button, like is on the top of a profile.

@cupOJoseph
Copy link
Contributor Author

@danlipert Just a friendly reminder that this is ready for a review again.

@PixelantDesign
Copy link
Contributor

yay! excited for this!

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.

left some comments

Comment on lines 89 to 111
$(elem).mouseenter(function(e) {
if ($(elem).hasClass('btn-outline-gc-green')) {
$(elem).addClass('btn-gc-outline-red').text('Unfollow');
$(elem).removeClass('btn-outline-gc-green');
}
}).mouseleave(function(e) {
if ($(elem).hasClass('btn-gc-outline-red')) {
$(elem).removeClass('btn-gc-outline-red');
$(elem).addClass('btn-outline-gc-green').text('Following');
}
});

$(elem).focus(function(e) {
if ($(elem).hasClass('btn-outline-gc-green')) {
$(elem).addClass('btn-gc-outline-red').text('Unfollow');
$(elem).removeClass('btn-outline-gc-green');
}
}).focusout(function(e) {
if ($(elem).hasClass('btn-gc-outline-red')) {
$(elem).removeClass('btn-gc-outline-red');
$(elem).addClass('btn-outline-gc-green').text('Following');
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use multiple event to be more DRY .on( "mouseenter focus", fnc ...

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -4896,3 +4896,18 @@ class TribeMember(SuperModel):
max_length=20,
blank=True
)

@property
def mutual_follow(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

tribesmembers is used for users following but also for orgs tribes seems this will show tribes orgs as following

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a handy method for Tribe Members, this method will be used as predicate to know if the current pair (org, profile) follow mutually.

Comment on lines 4900 to 4913
@property
def mutual_follow(self):
return TribeMember.objects.filter(profile=self.org, org=self.profile).exists()

@property
def mutual_follower(self):
tribe_following = TribeMember.objects.filter(profile=self.profile).values('org')
return TribeMember.objects.filter(org=self.org, profile__in=tribe_following).exclude(profile=self.profile)

@property
def mutual_following(self):
tribe_following = TribeMember.objects.filter(org=self.profile).values('profile')
print(tribe_following)
return TribeMember.objects.filter(org__in=tribe_following, profile=self.org)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about this logic, if you already have those who already following you why not better cross both and have the result to avoid many queries?
Also I see a missing print there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whats the missing print? Is there a rule for printing objects here we have to follow?

Copy link
Contributor

Choose a reason for hiding this comment

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

done, only one query is executed on each of these methods using a subquery

<div class="mb-0 mt-1 col-5 d-flex flex-row align-items-center">
<div class="mr-5" style="height: fit-content">
<button class="btn btn-outline-gc-blue btn-sm flex-grow-1 font-smaller-5 position-relative quick-link" data-openchat="{{user.profile}}">
<i class="fas fa-comment-dots" data-placement="bottom" data-toggle="tooltip" data-html="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

lets move the tooltip to the button to show it on all the button hover not just the icon

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Comment on lines 111 to 126
{% if followers|length == 2 %}
<div style="width: 60px" class="d-flex flex-row align-items-center">
<img class="rounded-circle" style="height: 25px; width: 25px; margin-right: -12px " src="/dynamic/avatar/{{ followers.0.profile.handle }}" alt="">
<img class="rounded-circle" style="height: 25px; width: 25px;" src="/dynamic/avatar/{{ followers.1.profile.handle }}" alt="">
</div>
<p class="text-muted font-smaller-4 my-0">Followed by {{ followers.0.profile.handle }} and {{ followers.1.profile.handle }}</p>
{% endif %}
{% if followers|length >= 3 %}
<div style="width: 60px" class="d-flex flex-row align-items-center">
<img class="rounded-circle" style="height: 25px; width: 25px; margin-right: -12px" src="/dynamic/avatar/{{ followers.0.profile.handle }}" alt="">
<img class="rounded-circle" style="height: 25px; width: 25px; margin-right: -12px" src="/dynamic/avatar/{{ followers.1.profile.handle }}" alt="">
<img class="rounded-circle" style="height: 25px; width: 25px;" src="/dynamic/avatar/{{ followers.2.profile.handle }}" alt="">
</div>
<p class="text-muted font-smaller-4 my-0">Followed by {{ followers.0.profile.handle }}, {{ followers.1.profile.handle }} and {{ followers|length|add:-2}} others you follow</p>
{% endif %}
{% endwith %}
Copy link
Contributor

Choose a reason for hiding this comment

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

lets move this logic to a loop instead and limit the looping to be more DRY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this would be better as a loop

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Comment on lines 139 to 143
setInterval(function(){
if($ && $('.load-more').length && $(".load-more").isInViewport()){
$('.load-more').click();
}
}, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

how this work? doesn't seems to be any ajax request handling this and if checking each second to click the button? Or I am missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

borrowed from a different tab, I think tab_kudos.html. Possibly didn't connect it, will check that.

Copy link
Contributor

Choose a reason for hiding this comment

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

done


print(f'===== TRIBES COUNT: {TribeMember.objects.filter(org=request.user.profile)}')
Copy link
Contributor

Choose a reason for hiding this comment

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

missing print

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Comment on lines 2667 to 2674
context['org_kudos'] = profile.get_org_kudos
owned_kudos = profile.get_my_kudos.order_by('id', order_by)
sent_kudos = profile.get_sent_kudos.order_by('id', order_by)
kudos_limit = 8
context['kudos'] = owned_kudos[0:kudos_limit]
context['sent_kudos'] = sent_kudos[0:kudos_limit]
context['kudos_count'] = owned_kudos.count()
context['sent_kudos_count'] = sent_kudos.count()
Copy link
Contributor

Choose a reason for hiding this comment

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

why this context here instead on the general context? shouldn't be here the follow data you need only for that tab?

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -83,3 +83,59 @@ const tribeLeader = () => {
};

tribeLeader();

const newManageTribe = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this file is loaded in many pleases probably is better to move it to his own file or even loaded on that page only.

Copy link
Contributor

Choose a reason for hiding this comment

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

could we omit this at this moment? i'll move this in another PR due i'll be working this weeks on tribes functionality.

Comment on lines 113 to 139
$(elem).on('click', function(e) {
if (!document.contxt.github_handle) {
e.preventDefault();
_alert('Please login first.', 'error');
return;
}

$(elem).attr('disabled', true);
e.preventDefault();
const tribe = $(elem).data('tribe');
const url = `/tribe/${tribe}/join/`;
const sendJoin = fetchData (url, 'POST', {}, {'X-CSRFToken': $("input[name='csrfmiddlewaretoken']").val()});

$.when(sendJoin).then(function(response) {
$(elem).attr('disabled', false);
$(elem).attr('member', response.is_member);
if (response.is_member) {
$(elem).addClass('btn-outline-gc-green').removeClass([ 'btn-gc-blue', 'btn-gc-outline-red' ]).text('Following');
} else {
$(elem).removeClass([ 'btn-outline-gc-green', 'btn-gc-outline-red' ]).addClass('btn-gc-blue').text('Follow');
}
}).fail(function(error) {
$(elem).attr('disabled', false);
});
});
});
};
Copy link
Contributor

Choose a reason for hiding this comment

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

if you rebase this then you will see this file now have const followRequest = (handle, elem, cb, cbError) function is just the request with a callback you can use it in a way you will have the handle, elem, response and in that way you don't need to copy paste the request part.

Copy link
Contributor

Choose a reason for hiding this comment

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

done :)

@octavioamu octavioamu marked this pull request as draft April 21, 2020 22:59
@danlipert
Copy link
Contributor

@jschiarizzi I'm marking this as ready for review since you still have it set as a draft. Please remember to mark it ready so it notifies us when you finish a draft PR - thanks

@danlipert danlipert marked this pull request as ready for review April 22, 2020 13:56
@danlipert
Copy link
Contributor

@jschiarizzi actually nvm I just saw @octavioamu set it as draft - disregard my comment

@cupOJoseph
Copy link
Contributor Author

Thanks for the notes Octavio, will get on this shortly.

@thelostone-mc thelostone-mc marked this pull request as draft April 29, 2020 13:47
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.

I left some comments about some things doesn't seeems to be related to this PR

Comment on lines 1052 to 1096

.new_grant {
border-top: 6px solid rgba(13, 7, 100, 0.25);
}

.new_kudos {
border-top: 6px solid rgba(0, 163, 255, 0.25);
}

.new_bounty {
border-top: 6px solid rgba(62, 0, 255, 0.4);
}

.new_hackathon_project {
border-top: 6px solid rgba(15, 206, 124, 0.25);
}

.bg-gc-blue {
background-color: var(--gc-blue);
}

.bg-gc-green {
background-color: var(--gc-green);
}

.bg-light-blue {
background-color: #00A3FF;
}

.bg-gc-dark-blue {
background-color: #0D0764;
}

.card-ribbon {
margin-top: -14px;
right: -10px;
width: 110px;
margin-bottom: 10px;
position: absolute;
}

.logo-metacard {
max-height: 4rem;
max-width: 4rem;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what is all of this here? Is related ?

@@ -15,14 +15,8 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.

{% endcomment %}
{% load humanize i18n static %}
{% load humanize i18n static grants_extra %}
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about this related

@thelostone-mc thelostone-mc marked this pull request as ready for review April 29, 2020 14:04
@thelostone-mc thelostone-mc merged commit 9e9329f into gitcoinco:master Apr 29, 2020
@owocki
Copy link
Contributor

owocki commented Jun 2, 2020

@jschiarizzi any interest in working on this follow up ticket? #6755

@cupOJoseph
Copy link
Contributor Author

Looks like you already got to 6755. 🌮

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants