-
Notifications
You must be signed in to change notification settings - Fork 9
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
Bind filters & pagination with url query params #83
Conversation
atomiix
commented
May 5, 2022
Questions | Answers |
---|---|
Description? | This PR aims to bind filters and pagination with the query parameters of the url so you can share the url and use the browser history to the desired filter/pagination combination. |
Type? | improvement |
BC breaks? | no |
Deprecations? | no |
Fixed ticket? | N/A |
How to test? | Play with the filters and the pagination (page & rows per page), check that the url is updated and check that when you click the buttons "previous" or "next" of your browser, filters are updated and the list as well. |
Possible impacts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but I'm not the best frontend dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eyh @PrestaShop/prestashop-maintainers need a 2nd advice 😄 from my point of view it's OK
there's no QA label for the nightly board ? |
pages/index.vue
Outdated
if (this.pagination.descending === true) { | ||
delete pagination.descending | ||
} | ||
if (this.pagination.page === 1) { | ||
delete pagination.page | ||
} | ||
if (this.pagination.rowsPerPage === 20) { | ||
delete pagination.rowsPerPage | ||
} | ||
if (this.pagination.sortBy === 'date') { | ||
delete pagination.sortBy | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would invert this, in case it's not the value you want, add the property
delete
can be sometime a bit expensive to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick comment but looks fine to me! 👍