-
-
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
Load metrics for top nav asynchronously #1231
Load metrics for top nav asynchronously #1231
Conversation
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 like where this is going, but have some suggestions 👍🏻
jobs_count = number_to_human(GoodJob::Job.count) | ||
batches_count = number_to_human(GoodJob::BatchRecord.migrated? ? GoodJob::BatchRecord.all.size : 0) | ||
cron_entries_count = GoodJob::CronEntry.all.size | ||
render json: { jobsCount: jobs_count, batchesCount: batches_count, cronEntriesCount: cron_entries_count } |
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.
nice and simple 👍🏻
This is a personal preference: I'd like these values to be snake_case instead of camelCase
@@ -0,0 +1,19 @@ | |||
import { Controller } from "stimulus" | |||
export default class extends Controller { |
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'd like this Stimulus controller to be written generically as like "AsyncValuesController". I'm imagining,
It would take a url
value that corresponded to the value being fetched.
It would have value
targets with a key
attribute whose value corresponded to a key in the fetched json.
The resulting HTML would look something like this:
<div data-controller="async-values" data-async-values-url="...">
<div data-async-values-target="value" data-async-values-key="jobs_count"></div>
</div>
697fc64
to
4776ceb
Compare
@bensheldon I apologize for taking sooooo long to make these changes. Past month's been a little of a roller coaster. Could you look at the changes and let me know what you think? |
…ss count too with color
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 made a couple tweaks and this looks fantastic! Thank you so much for doing this.
PR addresses #1057.
I have only worked on the metrics in the top nav like you mention in the comment in that issue. I am looking into loading the metrics in the tab navs async too, but I'll wait until you review this implementation.