-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add support for live polling the dashboard #528
Conversation
Adds support for live polling the dashboard, adding replaceable content areas anywhere on the page, register event listeners which act upon the `GoodJob` javacript object, and a footer. ### Polling UX 1. Toggle the `Live Poll` checkbox in the navbar (every 5000ms by default) 2. Add a query parameter to the url `/good_job?poll=5000`. The integer (in ms) sets the poll interval. ### Register event listeners 1. Add `data-gj-action` attribute with the event name and the `GoodJob` function to call when the event is triggered. `change#togglePoll` => `element.addEventListener('change, GoodJob.togglePoll)` ### Adding replaceable content areas 1. Add the `data-gj-poll-replace` attribute to the element 2. Add a unique ID to the same element Fixes bensheldon#526
If you have live poll enabled, and are typing in an input, your changes to the input can be replaced, resulting in a janky experience.
Inserting script tags as HTML strings doesn't work without hacks. Instead just render the JSON as a data attr.
Once this is merged (with JS groundwork laid here) I can tackle #446 |
@danielwestendorf thanks! I'll review this PR later today. |
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.
Thanks for doing this. This is great!
- I like the footer with the timestamp and the positive message 👍
- Change: I'd like to organize the behavior(s) into ES6 classes, rather than a bare
GoodJob
object. So I imagine that would mean there would be aPoller
object with a constructor that does the binding to the dom/setup. - Change: I'd prefer to use the History API's
replaceState
method instead of localstorage. My preference is to hold as much state as possible either in the URL or in the DOM. - Bug: setting
?poll=
to anything that isn't a positive integer puts the fetching into a loop. I think it should validate that the input is a positive integer, and any other value disables the functionality.
Also, I saw your comment about #446. I was imagining that being done without much javascript (other than some light usage of UJS; e,g, wrapping the table in a form, and converting the existing row button_to
s to UJS links to avoid a form nested in another form), so I'm curious about how you see these two functions being related
I went with the object that was already defined in
Storing the polling enablement strictly in the URL disables polling when navigating between controller actions. As a sidekiq user I expect polling to stay on as I navigate around the GoodJob dashboard until I disable it.
Fixed!
It might not need much JS, but if it does, I would want to utilize the groundwork laid here. It sounds like you have a good javascript-less plan around that implementation though. |
@danielwestendorf thanks and thanks for the feedback too! I'll push up those structural changes and get this merged. I really appreciate it. |
…ds; document `?poll` param in Readme
Adds support for live polling the dashboard, adding replaceable content areas anywhere on the page, register event listeners which act upon the
GoodJob
javacript object, and a footer.Polling UX
Live Poll
checkbox in the navbar (every 5000ms by default)/good_job?poll=5000
. The integer (in ms) sets the poll interval.Register event listeners
data-gj-action
attribute with the event name and theGoodJob
function to call when the event is triggered.change#togglePoll
=>element.addEventListener('change, GoodJob.togglePoll)
Adding replaceable content areas
data-gj-poll-replace
attribute to the elementFixes #526