-
Notifications
You must be signed in to change notification settings - Fork 30
Add pagination to activity lists #1681
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
Conversation
9a6cd09 to
f049159
Compare
| a.gap { | ||
| cursor: default; | ||
| text-decoration: none; | ||
| pointer-events: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to prevent the '...' in the pagination interface from being underlined/highlighted on mouseover
f049159 to
46504ce
Compare
| %div.m_10[l, :metadata] | ||
| = render :partial => 'locations/render_update_metadata', :locals => {:l => l} | ||
| - if ENV['AWS_BUCKET_NAME'] && !is_bot? | ||
| - if ENV['AWS_BUCKET_NAME'].present? && !is_bot? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting a lot of errors in my local env related to this, so I just went and made it optional
fac29e9 to
888ef92
Compare
| // Scroll to the top of the activity content with offset for fixed headers (220px) | ||
| let element = $('#recent_location_activity_location_#{location.id}')[0]; | ||
| let elementPosition = element.getBoundingClientRect().top; | ||
| let offsetPosition = elementPosition + window.pageYOffset - 220; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the hardcoded 220 here, but it works well enough.
Refactor with location activity Move javascript to be closer to its friends Fix nearby activity view closing upon pagination
888ef92 to
e96646d
Compare
|
Thanks. Will review after the holiday. Once again in the middle of a feature update, but hope to have it done soon. So far we're happy with not adding pagination to the urls Re: the |
| - if (!activity.user_name.blank?) | ||
| by | ||
| %span.bold #{activity.user_name} | ||
| .activity_hr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hr should be outside of the recent_activity_container
|
I'm not yet sure why, but in local dev, when I click "nearby locations" it centers me around Pittsburgh (which is fine), and then I click "nearby activity" it returns EDIT: Actually, I'm also seeing this on current upstream/master. So, not something introduced here. Might be a misunderstanding of something on my part. I'll look closer later. |
Ok, nevermind. Seems like it's a local data issue where I didn't populate lat/lon for many user submissions in my dev db. Sorry for noise. Anyway, this looks good so far to me. I will take another glance tonight or tomorrow and merge. I can adjust some style things. Thanks! |
|
Thanks for the merge! Did you want me to put up another PR that fixes the |
|
No, I adjusted the hr in a subsequent commit. Thanks for adding the pagination! Nice improvement. |
Fixes #1673.
This adds a pagination UI to "Nearby Activity" and to "Location Activity"
...
This PR also includes pulling out app/views/shared/_activity_item.html.haml into a separate file to avoid code duplication.
A few things to consider before merging:
?page=parameter to the URL bar (although this opens up quite a few routing questions that are beyond the scope of this PR)