Admin status page's current tab does not preserve #4299
Conversation
| @@ -49,7 +49,7 @@ export function WorkersTable({ loading, items }) { | |||
| dataSource={items} | |||
| pagination={{ | |||
| defaultPageSize: 25, | |||
| pageSizeOptions: [10, 25, 50], | |||
| pageSizeOptions: ['10', '25', '50'], | |||
There was a problem hiding this comment.
This is unrelated to this PR, I was just bothered by the console warning.
| const { isLoading, error, queueCounters, startedJobs, overallCounters, workers } = this.state; | ||
| const { isLoading, error, queueCounters, startedJobs, overallCounters, workers, activeTab } = this.state; | ||
|
|
||
| const changeTab = (newTab) => { |
There was a problem hiding this comment.
This is the simplest way I could find that doesn't involve bringing in a router.
Bring it on.
There was a problem hiding this comment.
What do you mean by bringing in a router?
There was a problem hiding this comment.
Directly manipulating the location hash isn't such a good practice. The appropriate way would probably be to define routes for these sub-pages, using something like react-router (which might not be an entirely bad idea, as it will open the door to more routing in the app, and, subsequently, the gate to hell)
There was a problem hiding this comment.
When we get rid of Angular we should look into having a separate URL for each of them, but right now it might prove to be tricky (without reloading all the data on every URL change).
But check how we handle hash navigation on the query page (we use it to direct link to specific visualizations) and see if it's not too complex. Or is it what you already do here?
There was a problem hiding this comment.
If you're referring to this, I think it'll be overkill as the admin page is almost entirely Angular-free. @kravets-levko WDYT?
There was a problem hiding this comment.
Oh, not that monstrosity. I'm not even sure why we need it there 😳 What you did is actually fine, let's roll with it.
Just to make sure: location.replace won't make Angular trigger a reload or something?
There was a problem hiding this comment.
It doesn't trigger a page reload, but it does trigger a component remounting, which in turns fetches new information from the API (not sure if that's really a bad thing).
There was a problem hiding this comment.
I was hoping to avoid that, to keep switching tabs snappy. Otherwise we could just use separate URLs (/admin/rq/queues, /admin/rq/workers, etc) instead.
I guess that to avoid remounting, we have to move fetching one level up and pass it down to the tabs (or have a "service" class that provides this state).
There was a problem hiding this comment.
It still is snappy. It switches instantaneously, but it triggers a background reload of data, which means that when you switch to Workers, for example, you will see data immediately, and a few hundred milliseconds later that data may change.
There was a problem hiding this comment.
Oh, ok. Not ideal but let's move on.
When viewing the Celery/RQ admin status pages (at
/admin/queries/jobsand/admin/queries/tasks), there are several tabs (for Queues, Workers, Other Tasks) that are not part of the URL, so whenever you visit the page, it defaults to the first tab.