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

Changed delete conversation button tooltip #5477

Merged
merged 1 commit into from
Dec 20, 2014
Merged

Changed delete conversation button tooltip #5477

merged 1 commit into from
Dec 20, 2014

Conversation

margori
Copy link
Contributor

@margori margori commented Dec 15, 2014

Delete conversation button currently says 'delete and block conversation'.

When I click that button I expect to delete or block that conversation from DB so that neither me or any participant can see it or reply.

Currently, signed in user is deleted from visibility table, so he is not longer part of the conversation.

"Leave conversation" is an appropriate text to describe button behavior.

Some localization files are update as well.

@jhass
Copy link
Member

jhass commented Dec 15, 2014

Please only edit the English locale, see https://wiki.diasporafoundation.org/Contribute_translations

@goobertron
Copy link

"Leave conversation" is an appropriate text to describe button behavior.

I agree, that's a far better word for this function. Although perhaps 'Mute conversation' would be more accurate, because (as I understand it) you are still included as part of the conversation from the other participants' point of view, it's just that you won't see any future replies. I'm happy with 'Leave conversation', although I'm not sure it's completely accurate for the current behaviour.

@margori
Copy link
Contributor Author

margori commented Dec 15, 2014

I use 'Leave conversation' because once you click the button, conversation is no longer shown in your conversation list.

@margori margori force-pushed the Leave_conversation_tooltip branch from 52f0f83 to e1cb98d Compare December 15, 2014 19:59
@margori
Copy link
Contributor Author

margori commented Dec 15, 2014

Please only edit the English locale

Done.

@svbergerem
Copy link
Member

'leave conversation' sounds to me like you can still see the old messages. (the same applies to 'Mute conversation')

@margori
Copy link
Contributor Author

margori commented Dec 15, 2014

'leave conversation' sounds to me like you can still see the old messages

OK. Please, what tooltip do you suggest?

My big concern is about trust: when I read 'delete conversation', I expect that conversation to be deleted from server when all participants agree to delete it.

If it remains, then 'delete conversation' is a lie.

@jhass
Copy link
Member

jhass commented Dec 15, 2014

How about ignore?

@margori
Copy link
Contributor Author

margori commented Dec 15, 2014

How about ignore?

Better :-)

@margori margori force-pushed the Leave_conversation_tooltip branch from e1cb98d to e93a58f Compare December 15, 2014 20:49
@svbergerem
Copy link
Member

@margori I completely agree that the current tooltip is wrong and that we should change it. I just thought that 'leave conversation' could confuse users who don't expect that the button would remove the conversation from the front end.

How about 'hide' or 'hide and ignore' / 'hide and mute'?

@goobertron
Copy link

'Ignore' is neat; however I think I prefer 'hide and mute' as it spells out more explicitly what will happen: the current conversation will be hidden from your view and future posts will be muted from you. This makes really clear to users what this button does.

@margori margori force-pushed the Leave_conversation_tooltip branch from e93a58f to 0849459 Compare December 17, 2014 18:07
@margori
Copy link
Contributor Author

margori commented Dec 17, 2014

What about this?

  • if participants > 1 show 'hide and mute conversation'
  • If conversation.participants = 1 show 'delete conversation'

How this change should be tested?

@margori margori changed the title Changed delete conversation button tooltip to leave conversation Changed delete conversation button tooltip Dec 18, 2014
@margori margori force-pushed the Leave_conversation_tooltip branch 2 times, most recently from c78fc6e to 81327c3 Compare December 18, 2014 12:32
@svbergerem
Copy link
Member

@margori Sounds good. I think in that case you should also change https://github.com/diaspora/diaspora/blob/develop/app/controllers/conversation_visibilities_controller.rb#L13 so you will get different flash messages depending on the number of remaining participants. (>0: successfully hidden, else: successfully deleted)

@margori
Copy link
Contributor Author

margori commented Dec 18, 2014

Since Pull request #5478 was merge, delete conversation button has two effects over the conversation. That's why my previous comment. :)

@svbergerem Good idea! I'll use it.

@goobertron
Copy link

What about this?

  • if participants > 1 show 'hide and mute conversation'
  • If conversation.participants = 1 show 'delete conversation'

Yes, I like this. Thanks to #5478 (if I understand it correctly), that is a good solution.

@margori margori force-pushed the Leave_conversation_tooltip branch 2 times, most recently from 5bfa081 to ea8e7cd Compare December 19, 2014 15:18
@margori
Copy link
Contributor Author

margori commented Dec 19, 2014

I think in that case you should also change https://github.com/diaspora/diaspora/blob/develop/app/controllers/conversation_visibilities_controller.rb#L13 so you will get different flash messages depending on the number of remaining participants. (>0: successfully hidden, else: successfully deleted)

Implemented + Test.

What do you think?

svbergerem
svbergerem reviewed Dec 19, 2014
View reviewed changes
spec/controllers/conversation_visibilities_controller_spec.rb
@@ -33,5 +33,18 @@
delete :destroy, :conversation_id => @conversation.id
}.not_to change(ConversationVisibility, :count)
end

it 'return "hidden"' do
Copy link
Member

Choose a reason for hiding this comment

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

it 'returns "hidden"' do

(the 's' is missing :-P)

@margori margori force-pushed the Leave_conversation_tooltip branch from ea8e7cd to c34979d Compare December 19, 2014 16:04
@margori
Copy link
Contributor Author

margori commented Dec 19, 2014

's' added. :-P

svbergerem
svbergerem reviewed Dec 19, 2014
View reviewed changes
app/controllers/conversation_visibilities_controller.rb
if @vis.destroy
flash[:notice] = I18n.t('conversations.destroy.success')
if (participants == 1)
Copy link
Member

Choose a reason for hiding this comment

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

please remove the round brackets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C# syntax... my bad :P

@margori margori force-pushed the Leave_conversation_tooltip branch from c34979d to 27a1886 Compare December 19, 2014 21:27
@svbergerem svbergerem merged commit 27a1886 into diaspora:develop Dec 20, 2014
svbergerem pushed a commit that referenced this pull request Dec 20, 2014
Merge pull request #5477 from margori/Leave_conversation_tooltip
1efed0e 
Changed delete conversation button tooltip
@svbergerem svbergerem added this to the next-major milestone Dec 20, 2014
@svbergerem
Copy link
Member

Thank you!

@Flaburgan
Copy link
Member

Thanks!

@margori margori deleted the Leave_conversation_tooltip branch December 22, 2014 13:17
@margori
Copy link
Contributor Author

margori commented Dec 22, 2014

You're very welcome!

@margori margori mentioned this pull request Dec 30, 2014
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@margori @jhass @goobertron @svbergerem @Flaburgan