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

Feature #5350 : allow user to enable / disable notifications for a post from stream #5511

Merged
merged 1 commit into from Feb 27, 2015

Conversation

Projects
None yet
8 participants
@collimarco
Copy link
Contributor

collimarco commented Dec 31, 2014

No description provided.

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Jan 23, 2015

Like the idea, but unfortunately it didn't seem to work on my dev box. I have two users, both sharing, two posts with other clicked participate in the other post (without commenting or liking), then commented as the post author. No notifications were generated.

Also, styling for the default icon seems wrong, it's almost invisible on hover - to cut participation seems fine.
selection_039
selection_040

@@ -16,6 +16,15 @@
<a href="#" rel="nofollow" class="block_user" title="{{t "ignore"}}">
<div class="icons-ignoreuser control_icon"></div>
</a>
{{#if participation}}
<a href="#" rel="nofollow" class="destroy_participation" title="{{t ""}}">

This comment has been minimized.

@jaywink

jaywink Jan 23, 2015

Contributor

Titles should be filled with a translation. Same in else.

@collimarco

This comment has been minimized.

Copy link
Contributor

collimarco commented Jan 24, 2015

@jaywink Added the missing part 56eb68e. Everything should be ok now.

For the translations I've notice that in general there's no correspondence between the key used in the template and the definitions in config/locales/diaspora/*.yml. I mean, the Rails convention is that when you find something like t "stream.hide" you expect to find stream: hide: "Hide" somewhere in the locales. This is not the case. So I cannot add them unless you tell me how you manage them. Online? Consider that this is a new translation to be added and not an existing one to be translated in other languages.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Jan 24, 2015

It does work like the rails convention, which is actually just how the i18n gem works, for the ones used in the client side JS you just have to put them under javascript, {{t "foo.bar"}} becomes javascript: foo: bar: "quux" in config/locales/javascript/javascript.en.yml.

@collimarco

This comment has been minimized.

Copy link
Contributor

collimarco commented Jan 25, 2015

@jhass Oops… you're right! I've added the missing translations to the en locale.

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Jan 25, 2015

Tested locally and the subscribe/unsubscribe works now perfectly - got notif without having to comment, great!

Only thing, the icons are still not visible properly.

Not subscribed, not hovering over icon:
selection_041

Not subscribed, hovering over icon:
selection_042

Subscribed, not hovering over icon:
selection_043

Subscribed, hovering over icon:
selection_044

Compare to ignore icon, not hovering over:
selection_045

And hovering over:
selection_046

Soooo, the icon needs to follow same guidelines, ie light gray when hovering over post but not icon and darker when hovering over it.

Also, can you squash commits to one for a cleaner commit log since this is all one feature?

Thanks, this feature is very needed! :)

@collimarco collimarco force-pushed the collimarco:issue5350 branch from 9cba7b5 to 09dc620 Jan 25, 2015

@collimarco

This comment has been minimized.

Copy link
Contributor

collimarco commented Jan 25, 2015

@jaywink I've improved the icons so that they are consistent with the others.

I've also done the requested squash with rebase -i.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Jan 25, 2015

Thanks for this, @collimarco. Are the tooltips styled as per @jaywink's screen shot? If so, they should be styled the same as the tooltip for the ignore icon. Apologies if I'm talking rubbish; I haven't had chance to pull and test locally, so just wanted to ask.

@collimarco collimarco force-pushed the collimarco:issue5350 branch from 09dc620 to eccab62 Jan 25, 2015

@collimarco

This comment has been minimized.

Copy link
Contributor

collimarco commented Jan 25, 2015

@goobertron Added the tooltip and squashed.

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor

SansPseudoFix commented Jan 25, 2015

Yeah, thanks @collimarco! Really usefull feature :)

@jhass jhass merged commit eccab62 into diaspora:develop Feb 27, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@jhass

This comment has been minimized.

Copy link
Member

jhass commented Feb 27, 2015

Sorry for letting this hang on for so long, let's see how badly this bloats the participations table further :P

I took the liberty to apply some fixes to this myself, in 073e999.

Thanks!

@jhass jhass added this to the next-major milestone Feb 27, 2015

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Feb 27, 2015

Yeah! Long missed feature appearing again, thank you! :)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 27, 2015

thank you @collimarco!

@Elm7

This comment has been minimized.

Copy link

Elm7 commented Mar 18, 2015

Hi ! Thanks alot for this feature. This is very handy.
Haven't seen that on Loomio, so I jump in there. Do you think this could be easily adapted to have the same button and feature on people profile page : This would then enable notifications for all new (visible by me) posts that this person will make ?

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Mar 18, 2015

@Loelo hey! This is a nice idea, but it's something completely new, so please open another issue about it.

@Elm7

This comment has been minimized.

Copy link

Elm7 commented Apr 15, 2015

Hello @fla, I hope it was the right way (no discussion on loomio first and because I am new to github), but I opened an issue here : #5860

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment