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.

Sign up for GitHub

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

Jump to bottom

Admin status page's current tab does not preserve #4299

Merged
merged 2 commits into from
Nov 11, 2019
Merged

Admin status page's current tab does not preserve #4299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c33d8fc
handle a console warning about passing in string page options
Nov 10, 2019
0390c7e
preserve selected tab in the location hash
Nov 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/app/components/admin/RQStatus.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function WorkersTable({ loading, items }) {
dataSource={items}
pagination={{
defaultPageSize: 25,
pageSizeOptions: [10, 25, 50],
pageSizeOptions: ['10', '25', '50'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to this PR, I was just bothered by the console warning.

showSizeChanger: true,
}}
/>
Expand Down
10 changes: 8 additions & 2 deletions client/app/pages/admin/Jobs.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import moment from 'moment';

class Jobs extends React.Component {
state = {
activeTab: location.hash.replace('#', '') || null,
isLoading: true,
error: null,

Expand Down Expand Up @@ -67,7 +68,12 @@ class Jobs extends React.Component {
};

render() {
const { isLoading, error, queueCounters, startedJobs, overallCounters, workers } = this.state;
const { isLoading, error, queueCounters, startedJobs, overallCounters, workers, activeTab } = this.state;

const changeTab = (newTab) => {
Copy link
Contributor Author

@rauchy rauchy Nov 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the simplest way I could find that doesn't involve bringing in a router.

Bring it on.

giphy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by bringing in a router?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're referring to this, I think it'll be overkill as the admin page is almost entirely Angular-free. @kravets-levko WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok. Not ideal but let's move on.

location.replace(`${location.pathname}#${newTab}`);
this.setState({ activeTab: newTab });
};

return (
<Layout activeTab="jobs">
Expand All @@ -87,7 +93,7 @@ class Jobs extends React.Component {
</Grid.Col>
</Grid.Row>

<Tabs defaultActiveKey="queues" animated={false}>
<Tabs activeKey={activeTab || 'queues'} onTabClick={changeTab} animated={false}>
<Tabs.TabPane key="queues" tab="Queues">
<QueuesTable loading={isLoading} items={queueCounters} />
</Tabs.TabPane>
Expand Down