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

#4304 Add ignore user icon on profile sidebar #4417

Conversation

@sjain1107
Copy link
Contributor

sjain1107 commented Aug 21, 2013

No description provided.

@Raven24
Copy link
Member

Raven24 commented Aug 21, 2013

[work in progress]


= link_to "#", :class => "block_user", :rel => "nofollow", :title => "ignore", :id => "block_user", :data => { :person_id => @person.id } do
.icons-ignoreuser.control_icon
:javascript

This comment has been minimized.

@svbergerem

svbergerem Aug 22, 2013 Member

I think this should be in app/assets/javascripts/people.js

This comment has been minimized.

@jhass

jhass Aug 23, 2013 Member

I'd vote for app/assets/javascripts/profile.js

This comment has been minimized.

@svbergerem

svbergerem Aug 23, 2013 Member

You are right. In #4374 I added some lines to people.js but they should be in profile.js, too. Right?

This comment has been minimized.

@jhass

jhass Aug 23, 2013 Member

Hm, yeah, looks like the better place to me actually :)

@@ -15,6 +15,24 @@
= link_to t('people.show.mention'), new_status_message_path(:person_id => @person.id), :class => 'button', :rel => 'facebox'
- if @contact.mutual?
= link_to t('people.show.message'), new_conversation_path(:contact_id => @contact.id, :name => @contact.person.name), :class => 'button', :rel => 'facebox'


= link_to "#", :class => "block_user", :rel => "nofollow", :title => "ignore", :id => "block_user", :data => { :person_id => @person.id } do

This comment has been minimized.

@svbergerem

svbergerem Aug 22, 2013 Member

I think this could be much easier to implement with a helper in app/helpers/people_helper.rb like I did here: https://github.com/diaspora/diaspora/pull/4374/files (add an block button unless the user is already blocked. In that case add an unblock button)

@jhass
jhass reviewed Aug 23, 2013
View changes
app/assets/stylesheets/application.css.sass Outdated
:left 0px
:padding
:bottom 12px

This comment has been minimized.

@jhass

jhass Aug 23, 2013 Member

Not sure how you managed to get these changes into your diff, easiest is probably redoing the changes in a clean branch:

git remote add upstream git://github.com/diaspora/diaspora.git
git fetch upstream
git branch -m 4304-add-ignore-user-icon 4304-add-ignore-user-icon_bak
git branch -m develop develop_bak
git checkout -b develop upstream/develop
git checkout -b 4304-add-ignore-user-icon
# Now apply your changes again
git commit -am "some descriptive message"
git push -uf origin develop 4304-add-ignore-user-icon
@jhass
jhass reviewed Aug 23, 2013
View changes
app/views/people/_profile_sidebar.html.haml Outdated

block.save({block : {person_id : personId}}, {
success : function(){
if(!app.stream) { return }

This comment has been minimized.

@jhass

jhass Aug 23, 2013 Member

No need for the success callback if you're not doing anything in this.

@jhass
jhass reviewed Aug 23, 2013
View changes
app/views/people/_profile_sidebar.html.haml Outdated
:javascript
$(document).ready(function() {
$("#block_user").click(function(evt) {
if(window.confirm("Ignore this user?"))
});
return false;
});
});
//.profile_button
// = link_to content_tag(:div, nil, :class => 'icons-ignoreuser', :title => t('people.show.message'), :id => 'block_user_button'), '/block', :rel => 'facebox'

This comment has been minimized.

@svbergerem

svbergerem Aug 23, 2013 Member

Those two lines displayed the ignore button in my PR. The best way to apply your change would be to remove the two slashes in front of the first line and replace the second line with the code you added above those lines (pay attention to the indentation).

@@ -18,11 +18,29 @@

- if @contact.mutual?
// remove the following line when adding the ignore button
.white_bar


.profile_button
= link_to content_tag(:div, nil, :class => 'icons-message', :title => t('people.show.message'), :id => 'message_button'), new_conversation_path(:contact_id => @contact.id, :name => @contact.person.name), :rel => 'facebox'
//.white_bar

This comment has been minimized.

@svbergerem

svbergerem Aug 23, 2013 Member

You should remove the two slashes here. I commented this line out to remove the ignore button which you would like to add again. There is also another line where you will have to do the same thing.

var block = new app.models.Block();

block.save({block : {person_id : personId}}, {
success : function(){

This comment has been minimized.

@jhass

jhass Aug 23, 2013 Member

Just remove the whole callback if you don't need it: block.save({block: {person_id: personID}});


= link_to "#", :class => "block_user", :rel => "nofollow", :title => "ignore", :id => "block_user", :data => { :person_id => @person.id } do
.icons-ignoreuser.control_icon
:javascript

This comment has been minimized.

@jhass

jhass Aug 23, 2013 Member

This block still should go to app/assets/javascripts/profile.js I think.

This comment has been minimized.

@sjain1107

sjain1107 Aug 24, 2013 Author Contributor

@MrZyx : Do you mean the code under :javascript?

This comment has been minimized.

//.profile_button
// = link_to content_tag(:div, nil, :class => 'icons-ignoreuser', :title => t('people.show.message'), :id => 'block_user_button'), '/block', :rel => 'facebox'
.profile_button
= link_to content_tag(:div, nil, :class => 'icons-ignoreuser block_user', :title => 'ignore', :id => 'block_user_button'), '#', :rel => "nofollow", :data => { :person_id => @person.id }

This comment has been minimized.

@svbergerem

svbergerem Aug 25, 2013 Member

:data => ... has to be moved into the content_tag(...), otherwise data('person-id') will be empty.

:title => 'ignore' should be translatable so I'd prefer :title => t('ignore')

This comment has been minimized.

@sjain1107

sjain1107 Aug 25, 2013 Author Contributor

:title => t('ignore') is displaying this

image

This comment has been minimized.

@svbergerem

svbergerem Aug 25, 2013 Member

Oh you are right! Then I think one would have to add it to config/locales/diaspora/en.yml
I think you could add it to the first section. There are other words like "Cancel", "Delete" and "Hide", so I think "Ignore" would fit in.

This comment has been minimized.

@sjain1107

sjain1107 Aug 25, 2013 Author Contributor

@svbergerem The refined one 👍

image

This comment has been minimized.

@svbergerem

svbergerem Aug 25, 2013 Member

nice :)

if(!confirm(Diaspora.I18n.t('ignore_user'))) { return }
var personId = $(this).data('person-id');
var block = new app.models.Block();

This comment has been minimized.

@svbergerem

svbergerem Aug 25, 2013 Member

the block.save({.. part is missing here.

@svbergerem
Copy link
Member

svbergerem commented Aug 25, 2013

@MrZyx The javascript is not being invoked in profile.js while it's working fine in people.js. Any ideas why this doesn't work?

@sjain1107 You will also have to change app/assets/stylesheets/profile.css.scss. There are two comments (those are the only comments ;) ) you will have to uncomment and delete in both cases the line under the comment.

@jhass
Copy link
Member

jhass commented Aug 25, 2013

Hm, looks like profile.js is only included on the profile edit page, on the profile/person show page people.js is explicitly included. So maybe we just keep it that way then and keep/move the JS to people.js

@sjain1107
Copy link
Contributor Author

sjain1107 commented Aug 25, 2013

@MrZyx If I add the JS to people.js, the person once ignored can be re-ignored !!
image

@jhass
Copy link
Member

jhass commented Aug 25, 2013

Don't display the icon if there's already a block then. I doubt this changes in any way if you inline the script or put it into people.js

@goobertron
Copy link

goobertron commented Aug 25, 2013

The original idea, as I understood it, was for it to be possible to ignore a user from the profile page, and to stop ignoring a user if they are currently being ignored.

We will need a specific area on the red bar (when a user is currently being ignored) where you can click to stop ignoring that user. It would be useful to have an icon in this area, to make it obvious what to click to stop ignoring that person. Someone suggested using the standard 'ignore' icon but without the line through the person; if that's not possible, perhaps we should use the same icon as for 'ignore' - the bar being red is the key that you will stop ignoring the person by clicking the icon. And perhaps a tooltip could read 'ignore this user' or 'stop ignoring this user', depending on the case.

Or is the option to stop ignoring a user from the profile page being dropped from this PR?

@svbergerem
Copy link
Member

svbergerem commented Aug 25, 2013

@goobertron I'd vote for "drop that feature from this PR, try to get this PR done for the release and implement the 'stop ignoring' in a different PR"

@DeadSuperHero
Copy link
Member

DeadSuperHero commented Aug 27, 2013

Seems we have one failing cucumber spec for blocking the user from the profile page. Do we have time to fix that so that it can make it in to the release? :)

@PallaviTS
Copy link
Contributor

PallaviTS commented Aug 27, 2013

We will work on it :) Try our best.

On Tue, Aug 27, 2013 at 12:45 PM, Sean Tilley notifications@github.comwrote:

Seems we have one failing cucumber spec for blocking the user from the
profile page. Do we have time to fix that so that it can make it in to the
release? :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/4417#issuecomment-23317058
.

Regards ,
Pallavi TS

@Raven24
Copy link
Member

Raven24 commented Aug 29, 2013

do we need the $("#block_user_button").click function in both profile.js and people.js?

@jhass
Copy link
Member

jhass commented Aug 29, 2013

do we need the $("#block_user_button").click function in both profile.js and people.js?

Nope, just people.js, profile.js is for the edit profile page actually.


$(document).ready(function() {
});

This comment has been minimized.

@Raven24

Raven24 Aug 29, 2013 Member

if it's empty like here, you can remove the two lines completely ;)

@Raven24
Copy link
Member

Raven24 commented Aug 30, 2013

ok, so I sent you another pull request on your branch. that is the last thing I think is necessary before we can pull this.
Please rebase again and add a changelog entry once you included the changes.

(Also, please always comment on the pull request, when you update the branch. Otherwise we don't get any notifications that something was changed.)

@sjain1107
Copy link
Contributor Author

sjain1107 commented Aug 31, 2013

I have added changelog entry.

@sjain1107
Copy link
Contributor Author

sjain1107 commented Sep 1, 2013

Final commits.

@@ -370,6 +370,7 @@ everything is set up.
* Show the service username in a tooltip next to the publisher icons [#4126](https://github.com/diaspora/diaspora/pull/4126)
* Ability to add location when creating a post [#3803](https://github.com/diaspora/diaspora/pull/3803)
* Added oEmbed provider for MixCloud. [#4131](https://github.com/diaspora/diaspora/pull/4131)
* Added ignore user icon. [#4417](https://github.com/diaspora/diaspora/pull/4417)

This comment has been minimized.

@jhass

jhass Sep 1, 2013 Member

This is still the wrong place. Add it to the Head section.

This comment has been minimized.

@sjain1107

sjain1107 Sep 1, 2013 Author Contributor

Head

  • Added ignore user icon #4417

Refactor

After that should I use git push -f origin 4304-add-ignore-user-icon ?

This comment has been minimized.

@jhass

jhass Sep 1, 2013 Member

Move it, don't add it to both.

This comment has been minimized.

@sjain1107

sjain1107 Sep 4, 2013 Author Contributor

@MrZyx Shall I change the file(changelog.md) directly on Github from my repo instead of commiting using shell commands?

This comment has been minimized.

@jhass

jhass Sep 4, 2013 Member

Doesn't matter at all. Just remove that line.

@@ -1,4 +1,5 @@
# Head
* Added ignore user icon [#4417](https://github.com/diaspora/diaspora/pull/4417)

This comment has been minimized.

@jhass

jhass Sep 1, 2013 Member

Head section but Features subsection.

sjain1107 referenced this pull request in railsgirls-generator-app/diaspora Sep 1, 2013
@Raven24
Copy link
Member

Raven24 commented Sep 1, 2013

I'll take care of this and remove that extra commit (later, I'm gonna be afk for the afternoon)

@sjain1107
Copy link
Contributor Author

sjain1107 commented Sep 4, 2013

Some tests are failing.

@jhass
Copy link
Member

jhass commented Sep 4, 2013

I squashed the commits a bit together that's why Github doesn't show this as merged. Thank you!

@jhass jhass closed this Sep 4, 2013
@goobertron
Copy link

goobertron commented Sep 4, 2013

Well done Sakshi, Pallavi and everyone else for pulling this together!

@pxlpnk
Copy link

pxlpnk commented Sep 4, 2013

👍

@DeadSuperHero
Copy link
Member

DeadSuperHero commented Sep 4, 2013

Yay, great job! :D

On Wednesday, September 4, 2013, Andreas Tiefenthaler wrote:

[image: 👍]


Reply to this email directly or view it on GitHubhttps://github.com//pull/4417#issuecomment-23786728
.

@sjain1107
Copy link
Contributor Author

sjain1107 commented Sep 4, 2013

Thank u so much all :) Specially @MrZyx, @Raven24 and @svbergerem

@PallaviTS
Copy link
Contributor

PallaviTS commented Sep 4, 2013

Thank you all :)

On Wed, Sep 4, 2013 at 6:30 PM, Sakshi Jain notifications@github.comwrote:

Thank u so much all :) Specially @MrZyx https://github.com/MrZYX,
@Raven24 https://github.com/Raven24 and @svbergeremhttps://github.com/svbergerem


Reply to this email directly or view it on GitHubhttps://github.com//pull/4417#issuecomment-23786872
.

Regards ,
Pallavi TS

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.