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

Feat/job counts #1618

Closed
wants to merge 5 commits into from
Closed

Feat/job counts #1618

wants to merge 5 commits into from

Conversation

brosenberg42
Copy link
Member

@brosenberg42 brosenberg42 commented Nov 15, 2022

@brosenberg42 brosenberg42 self-assigned this Nov 15, 2022
Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


trunk/workflow-manager/src/main/webapp/resources/mytheme/js/angular_app/app.js line 69 at r1 (raw file):

	  $urlRouterProvider.otherwise("/jobs");

	  // Matches URLs with no fragment. After a user initally logs in, in the URL will not have a

I think you have an extra "in" in "in, in the URL".

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @brosenberg42)


trunk/workflow-manager/src/main/webapp/resources/mytheme/js/controllers/JobsCtrl.js line 317 at r1 (raw file):

        if (resp.hasMorePages) {
            // We don't know how many more pages there are, so just show the maximum number of
            // pagination links. If user clicks past the end we will just move them to the actual

It seems a little strange to say "Showing 51 to 59 of 114 total entries" when there are really only 59 filtered results. Can we just say "Showing X to Y entries"?

Also seems a little strange to click on page 12 and then end up on page 6 when looking at filtered results.

I wonder if it's better to just not show the total number of entries when doing a search and just have the user click "Next" through the pages one-by-one. That means not showing the final page after the "..." since we don't know what the right number should be (unless you can call it "Last" instead of a number).

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @jrobble)


trunk/workflow-manager/src/main/webapp/resources/mytheme/js/angular_app/app.js line 69 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I think you have an extra "in" in "in, in the URL".

Done.


trunk/workflow-manager/src/main/webapp/resources/mytheme/js/controllers/JobsCtrl.js line 317 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

It seems a little strange to say "Showing 51 to 59 of 114 total entries" when there are really only 59 filtered results. Can we just say "Showing X to Y entries"?

Also seems a little strange to click on page 12 and then end up on page 6 when looking at filtered results.

I wonder if it's better to just not show the total number of entries when doing a search and just have the user click "Next" through the pages one-by-one. That means not showing the final page after the "..." since we don't know what the right number should be (unless you can call it "Last" instead of a number).

The old message used to mention the total number of entries, so I tried to keep that behavior. The message does say "total".

One issue is that we can't change the pagination type specifically for when a filter is being applied. It is set when the table is initialized and can't be changed afterwards. Also, even when a filter is applied, being able to jump forward a few pages is useful.

@brosenberg42 brosenberg42 deleted the feat/job-counts branch December 12, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants