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

Paging should show total number of pages/records #153

Open
blcham opened this issue Apr 23, 2024 · 15 comments
Open

Paging should show total number of pages/records #153

blcham opened this issue Apr 23, 2024 · 15 comments
Assignees

Comments

@blcham
Copy link

blcham commented Apr 23, 2024

Image

The backend should return both:

  • total number of pages
  • total number of records

A/C:

  • total number of pages or total number of records or both are somehow accessible to user
  • same paging should be implemented for both "record items" and "history items"
@blcham
Copy link
Author

blcham commented Apr 23, 2024

Total count of records was implemented in kbss-cvut/record-manager#49

@blcham
Copy link
Author

blcham commented Apr 23, 2024

According to @ledsoft, it might be just a bug as it seems like already implemented.

@ledsoft
Copy link

ledsoft commented Apr 23, 2024

@LaChope I would in particular look into extracting page count from link headers (extractLastPageNumber in Utils), because we have verified that the server response does contain paging links.

@blcham blcham assigned shellyear and unassigned LaChope Apr 23, 2024
@shellyear
Copy link
Collaborator

shellyear commented Apr 23, 2024

@blcham, @ledsoft extractLastPageNumber is already being used in RecordActions.js >> loadRecords, and the function returns undefined for the pageCount

@ledsoft
Copy link

ledsoft commented Apr 23, 2024

Well, that's why I suggested looking into that function:) Have you found out why it returns undefined?

@shellyear
Copy link
Collaborator

shellyear commented Apr 23, 2024

Well, that's why I suggested looking into that function:) Have you found out why it returns undefined?

Not yet, but I think it relates to response.headers.link, which is "<http://record-manager-server:8080/record-manager?page=0&size=10>; rel="first", <http://record-manager-server:8080/record-manager?page=1&size=10>; rel="last", <http://record-manager-server:8080/record-manager?page=1&size=10>; rel="next""

For instance here is theresponse.headers.link
<http://record-manager-server:8080/record-manager?page=0&size=10>; rel="first", <http://record-manager-server:8080/record-manager?page=7&size=10>; rel="last", <http://record-manager-server:8080/record-manager?page=1&size=10>; rel="next" from the https://kbss.felk.cvut.cz/ava/failure-record-manager/records. So I assume I should pay attention for the link="last" and the page parameter for this particular link

@blcham
Copy link
Author

blcham commented Apr 23, 2024

@shellyear note, that i modified initial A/C, to implement also paging "history items":
image

@blcham
Copy link
Author

blcham commented Apr 23, 2024

@shellyear not sure, but it seems that bad "response.headers.link" is not problem for you to implement the ticket, right? If so, I suggest creating a separate ticket (will have low priority now) to handle this issue.

@ledsoft, any preference/suggestions on how to solve the issue with incorrect links, I guess alternatives are:

    1. relative links instead of absolute
    1. use additional environment variable to the backend (note that we already have the public address of the frontend there :( )

To me, i) seems a better variant, while later, we can implement ii) in cases when the environment variable is defined.

@ledsoft
Copy link

ledsoft commented Apr 24, 2024

@blcham @shellyear I see it more as a proxy configuration issue - the link URLs are correct w.r.t. the instance. Of course, generating relative links is also possible according to the RFC.

Nevertheless, the fact that the links point to the internal docker URL of the service should not prevent fixing the issue. We are interested in the last link and in it, we are interested in the page query attribute. Neither of these depend on the absolute URL of the links.

@shellyear
Copy link
Collaborator

@blcham
Copy link
Author

blcham commented Apr 24, 2024

@shellyear
Just additional notes to my proposal. Let's assume we have 10 pages and we will show next 2 pages from the current one. The UI would look like as follows:

- at page 1:    << < "1" 2 3 ... 10 >
- at page 2:    << < "2" 3 4 ... 10 >
- at page 3:    << < "2" 3 4 ... 10 >
- at page 7:    << < "7" 8 9 10 >
- at page 8:    << < "8" 9 10 >
- at page 9:    << < "9" 10 >
- at page 10:   << < "10" >

@blcham
Copy link
Author

blcham commented Apr 24, 2024

@blcham https://codepen.io/josetxu/pen/vYvgJVE

It looks good, but what I don't like there is that you never know how many pages are there until you navigate to the last page.

@ledsoft
Copy link

ledsoft commented Apr 25, 2024

Now that you have the total count available in the X-Total-Count header, you can show it either near the paging component or (as TermIt has it) in the filter input at the top of the table (Filter n records).

@shellyear
Copy link
Collaborator

shellyear commented Apr 25, 2024

@blcham Do we want the ellipsis to be clickable (expandable, so the other page numbers will be shown) like here ? https://codepen.io/dmytroyarmak/full/VagMQq/

@blcham
Copy link
Author

blcham commented Apr 26, 2024

I have the following notes:

  1. not sure, but I think we do not have bootstrap-4 in the record manager
  2. "..." seems to me not intuitive
    image

Thus make it clickable if you think it makes sense but does not cost more than 5 mins to implement.

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

No branches or pull requests

4 participants