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

Increased the number of notifications shown in drop down bar to 15 #5129

Merged
merged 1 commit into from Aug 26, 2014

Conversation

Projects
None yet
5 participants
@jaideng123
Copy link
Contributor

jaideng123 commented Aug 18, 2014

In response to the complain in issue #5106 I've modified the behavior of the notifications drop down to scroll when it overflows and increased the number of shown notifications from 5 to 15

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 18, 2014

Could you please add a screenshot of the dropdown with the scroll?

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 18, 2014

The scrollbar is also show if you have less than 5 notifications. I'm not sure if we want one at all. I'm pretty sure we don't wan't the native one, Gnome's integrates quite well but that's nothing to rely on.

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Aug 18, 2014

I'll look into some alternatives and get back to you with something a little cleaner

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Aug 19, 2014

here's a screenshot of it
screenshot from 2014-08-18 20 32 58

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 19, 2014

I'm still not a huge fan of that native scollbar there :/ What do others think?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 19, 2014

@jhass I agree. https://plugins.jquery.com/perfect-scrollbar/ could be the solution.

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Aug 19, 2014

perfect-scrollbar looks totally awesome - +1 for that

Thanks for this change @jaideng123 - hugely annoyed myself that we have the 5 notifs limit. Maybe later this can be improved further that it auto-loads more notifications as you scroll down, but this is a very good start to improve the situation.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 19, 2014

If you're going to integrate perfect-scollbar please use the bower package through https://rails-assets.org/

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Aug 20, 2014

I'll work on integrating perfect scroll, if I get around to adding infinite scrolling I'll probably make it a different PR

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 20, 2014

How does facebook and other website with notifications deal with that? I honnestly don't like scroll bars at all. Do we really want more notification here?

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 20, 2014

Loomio:

@jaideng123 jaideng123 force-pushed the jaideng123:5106-scrolling_in_notifications branch 5 times, most recently from 1f00d25 to 2ffae3d Aug 20, 2014

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 23, 2014

Hey, you should comment on the PR, not your commits, so we get notifications ;)

The Jasmine tests seem to be fixed by keeping the jquery dependencies in sync:

-  gem 'rails-assets-perfect-scrollbar'
+  gem 'rails-assets-perfect-scrollbar', '0.4.11'
+  # This should be kept in sync with jquery-rails
+  gem 'rails-assets-jquery', '1.10.2'

Make sure to commit the changes to Gemfile.lock. Never edit that manually, run bundle (with DB set to nothing or mysql) and commit the changes.

I'm not sure I can follow you on the bootstrap-sass / strong_parameters thing.

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Aug 23, 2014

Ohhh totally my bad, I'll post a screenshot of the overlap issue I'm having along with a more detailed explanation of the dependencies involved in updated to bootstrap-sass 2.3 later today

@jhass jhass added this to the next-major milestone Aug 24, 2014

@jaideng123 jaideng123 force-pushed the jaideng123:5106-scrolling_in_notifications branch 5 times, most recently from 001d24c to 04d281d Aug 25, 2014

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Aug 25, 2014

Alright i managed to get it all working here's a screenshot
screenshot from 2014-08-25 12 37 33

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 25, 2014

Maybe increase the padding a bit and make it a bit wider, the read toggle seems a bit close to the scrollbar for my taste :)

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Aug 25, 2014

Better?
screenshot from 2014-08-25 14 02 47

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 25, 2014

Yeah :)

@jaideng123 jaideng123 force-pushed the jaideng123:5106-scrolling_in_notifications branch from 04d281d to 3d2b9a9 Aug 25, 2014

@jaideng123 jaideng123 force-pushed the jaideng123:5106-scrolling_in_notifications branch from 3d2b9a9 to b70f306 Aug 25, 2014

@jhass jhass merged commit b70f306 into diaspora:develop Aug 26, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

jhass added a commit that referenced this pull request Aug 26, 2014

Merge pull request #5129 from jaideng123/5106-scrolling_in_notifications
Increased the number of notifications shown in drop down bar to 15

Conflicts:
	Gemfile.lock
@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 26, 2014

Thank you!

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 26, 2014

@jaideng123 Thank you. Just updated my pod and it looks great. On Bootstrap pages (notifications, conversations, settings, ...) the scroll bar is unfortunately missing (not visible) and when you scroll down there is some white space. It would be great if you could try to fix that.

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Aug 26, 2014

@svbergerem That's very strange, any idea what could be causing it to behave like that?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 26, 2014

@jaideng123 You imported the perfect-scrollbar css in application.css.sass which has no effect on Bootstrap pages so you should also import it in new-templates.cs.scss. I haven't tried if that fixes the whole problem but it is definitely part of it.

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