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

Auto refresh notifications page #200

Merged
merged 14 commits into from Jan 2, 2017

Conversation

@WillAbides
Copy link
Contributor

commented Dec 31, 2016

closes #198
no longer blocked by #199

This refreshes notifications on an interval set by the user

@WillAbides WillAbides changed the title [WIP] Auto refresh notifications page Auto refresh notifications page Jan 1, 2017

@WillAbides

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2017

I opted for the settings page to set the refresh interval in one minute increments so that people don't end up setting something ridiculously short.

@WillAbides WillAbides force-pushed the WillAbides:auto-refresh branch from f1cd9df to 7fefbc5 Jan 1, 2017

@@ -0,0 +1,5 @@
class AddDefaultValueToUsersRefreshInterval < ActiveRecord::Migration[5.0]
def change
change_column_null :users, :refresh_interval, false, 0

This comment has been minimized.

Copy link
@WillAbides

WillAbides Jan 1, 2017

Author Contributor

I'm not sure why this didn't set the default value to 0

This comment has been minimized.

Copy link
@tarebyte

tarebyte Jan 1, 2017

Member

I'm pretty sure it's default: 0

This comment has been minimized.

Copy link
@WillAbides

WillAbides Jan 2, 2017

Author Contributor

Looks like I had the syntax right, but it didn't do what I thought. That just changes existing rows to 0. The documentation even says "Please note the fourth argument does not set a column's default."

I just needed to RTFM

@WillAbides WillAbides force-pushed the WillAbides:auto-refresh branch from e08fccd to f19732a Jan 2, 2017

@WillAbides

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2017

I cleaned this up and force pushed so the commit log isn't so messy

@andrew andrew referenced this pull request Jan 2, 2017
@WillAbides

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2017

@andrew: Given your concerns in #205 about hitting the api too hard, maybe I should add a system config to enable this and set the minimum refresh time.

Something like MINIMUM_REFRESH_INTERVAL. If it is unset or 0, this is disabled. Otherwise it is the minimum number of minutes a user is allowed to set refresh_interval to.

@andrew

This comment has been minimized.

Copy link
Member

commented Jan 2, 2017

@WillAbides makes sense, I'm not going to be using this feature or #205 as I like to hit refresh once I'm done with everything I've already got in my inbox so would be good to get feedback from other users too

@WillAbides

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2017

I've now put this behind a system config. The default minimum refresh interval is 10 minutes.

I think this is ready for merging.

@andrew

This comment has been minimized.

Copy link
Member

commented Jan 2, 2017

@WillAbides just need to change the javascript_tag to be a bit more unobtrusive, i.e. move the function to be called after document.ready in application.js and check for the presence of a data attribute in the page with the value of the interval.

@WillAbides

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2017

@andrew: Is 0fdedf0 what you're looking for?

@andrew

This comment has been minimized.

Copy link
Member

commented Jan 2, 2017

@WillAbides Nearly there but rather than setting a js variable in the html, add data-refresh-interval=<%= current_user.effective_refresh_interval %> to the main <table> on the page, then load it in your setAutoSyncTimer function with $('.js-table-notifications').data('refresh-interval')

@WillAbides

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2017

@andrew: Maybe this is better

@andrew

This comment has been minimized.

Copy link
Member

commented Jan 2, 2017

👍

@andrew andrew merged commit 783a171 into octobox:master Jan 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@WillAbides WillAbides deleted the WillAbides:auto-refresh branch Jan 2, 2017

@johnthepink johnthepink referenced this pull request Jan 6, 2017
@WillAbides WillAbides referenced this pull request Jan 19, 2017
@chrisarcand chrisarcand referenced this pull request Aug 29, 2017
@Floppy Floppy referenced this pull request Oct 31, 2017
1 of 2 tasks complete
@andrew

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

FYI I've just enabled auto-refresh on octobox.io with a 5 minute minimum: f9b640e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.