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

2023.9.x feature add per page resource list #1534

Merged
merged 1 commit into from Nov 29, 2023

Conversation

EmmyMay
Copy link
Contributor

@EmmyMay EmmyMay commented Nov 22, 2023

The following changes are implemented

TODO: Summary

Changes in the user interface:

TODO: Add screenshots, recordings or remove this section

Checklist when submitting a final (!draft) PR

  • Commits are tidied up, squashed if needed and follow guidelines in CONTRIBUTING.md
  • Code builds
  • All existing tests pass
  • All new critical code is covered by tests
  • PR is linked to the relevant issue(s)
  • Rebased with the target branch
    closes Add per page dropdown to resource lists #1497

@EmmyMay EmmyMay requested a review from Fajfa November 22, 2023 16:47
@EmmyMay EmmyMay self-assigned this Nov 22, 2023
{{ getPagination }}
</div>

<div>
Copy link
Member

Choose a reason for hiding this comment

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

Where is the text like in recordList?

@@ -282,6 +292,7 @@ export default {
return {
selected: [],
selectableItemIDs: [],
rowsPerPage: 10,
Copy link
Member

Choose a reason for hiding this comment

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

Dont think we need this

@@ -408,6 +433,11 @@ export default {
this.$router.replace({ query: { ...this.$route.query, page, pageCursor } })
},

loadRows () {
Copy link
Member

Choose a reason for hiding this comment

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

You need to use the value you select, not the pagination limit


<div>
<b-form-select
v-model="pagination.limit"
Copy link
Member

Choose a reason for hiding this comment

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

Dont v-modal pagination.limit, the value should be updated by the query.replace. You only pass the value here.

<b-form-select
v-model="pagination.limit"
:options="perPageOptions"
@change="loadRows"
Copy link
Member

Choose a reason for hiding this comment

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

Lets call this by what it is please, like you did in recordList, onPerPageChange

</div>

<div>
<b-form-select
Copy link
Member

Choose a reason for hiding this comment

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

This should be togglable in a prop.
You need to amend showFooter aswell just like you did in recordList. Try it with different options, how does it look and work.

class="text-nowrap"
>
{{ getPagination }}
<div class="d-flex gap-1 align-items-center">
Copy link
Member

Choose a reason for hiding this comment

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

It should wrap like recordList, styles should be similar. User shouldn't see a difference

@@ -126,65 +126,72 @@
</b-card-body>

<template
v-if="showFooter"
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been removed?

@change="loadRows"
/>
<div class="d-flex flex-wrap gap-1 align-items-center">
<div>
Copy link
Member

Choose a reason for hiding this comment

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

We dont need this div 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.

putting it in span means I have to add a block class to it which is the same as having a div imo

@@ -433,8 +446,8 @@ export default {
this.$router.replace({ query: { ...this.$route.query, page, pageCursor } })
},

loadRows () {
this.$router.replace({ query: { ...this.$route.query, page: 1, limit: this.pagination.limit } })
lhandlePerPageChange (v) {
Copy link
Member

Choose a reason for hiding this comment

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

lhandle?

@@ -433,8 +446,8 @@ export default {
this.$router.replace({ query: { ...this.$route.query, page, pageCursor } })
},

loadRows () {
this.$router.replace({ query: { ...this.$route.query, page: 1, limit: this.pagination.limit } })
lhandlePerPageChange (v) {
Copy link
Member

Choose a reason for hiding this comment

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

call it limit so you dont have to define it as limit: v

<b-form-select
:options="perPageOptions"
:value="pagination.limit"
style="width: 70px;"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this, if so why is it not in recordList?

@EmmyMay EmmyMay force-pushed the 2023.9.x-feature-add-per-page-resource-list branch from bf89e86 to ad78724 Compare November 27, 2023 13:05
@@ -342,10 +384,11 @@ export default {

hasNextPage () {
return !!this.pagination.nextPage

Copy link
Member

Choose a reason for hiding this comment

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

Lint please, no need for this extra line and spaces

},

showFooter () {
return !(this.hideTotal && this.hidePagination)
return !(this.hideTotal && this.hidePagination && !this.hidePerPageOption)
Copy link
Member

Choose a reason for hiding this comment

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

Fix this please

@EmmyMay EmmyMay force-pushed the 2023.9.x-feature-add-per-page-resource-list branch from 0106d0b to c5caedd Compare November 28, 2023 12:05
@katrinDY
Copy link
Contributor

One small thing in reporter, else all good:

Screenshot 2023-11-29 at 13 10 59

@EmmyMay EmmyMay force-pushed the 2023.9.x-feature-add-per-page-resource-list branch from c5caedd to a2972c4 Compare November 29, 2023 12:54
@EmmyMay EmmyMay merged commit a2972c4 into 2023.9.x Nov 29, 2023
1 check passed
@EmmyMay EmmyMay deleted the 2023.9.x-feature-add-per-page-resource-list branch November 29, 2023 12:59
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.

Add per page dropdown to resource lists
3 participants