kbss-cvut / record-manager-ui

GNU Lesser General Public License v3.0
0 stars 2 forks source link

Paging should show total number of pages/records #153

Open blcham opened 5 months ago

blcham commented 5 months ago

Image

The backend should return both:

A/C:

blcham commented 5 months ago

Total count of records was implemented in https://github.com/kbss-cvut/record-manager/pull/49

blcham commented 5 months ago

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

ledsoft commented 5 months ago

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

shellyear commented 5 months ago

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

ledsoft commented 5 months ago

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

shellyear commented 5 months ago

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 commented 5 months ago

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

blcham commented 5 months ago

@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:

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

ledsoft commented 5 months ago

@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 commented 5 months ago

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

blcham commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

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.