-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor kotti pagination #543
Conversation
Your Render PR Server URL is https://kotti-pr-543.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c5jpedc6fj36arvjfvkg. |
7364b8a
to
fc8a44b
Compare
packages/kotti-ui/source/kotti-pagination/components/PaginationFlexible.vue
Outdated
Show resolved
Hide resolved
packages/kotti-ui/source/kotti-pagination/components/PaginationFractionated.vue
Outdated
Show resolved
Hide resolved
packages/kotti-ui/source/kotti-pagination/components/PaginationFlexible.vue
Outdated
Show resolved
Hide resolved
packages/kotti-ui/source/kotti-pagination/components/PaginationFlexible.vue
Outdated
Show resolved
Hide resolved
f388921
to
e5a2600
Compare
3e8d3bd
to
4deb70d
Compare
b1642f2
to
20f996e
Compare
This reverts commit 3f1bb38.
This reverts commit 9d09175.
This reverts commit 134d42b.
…s internally mutating the replies" This reverts commit ee74729.
This reverts commit e144bc5.
…o component (from _comments.scss)" This reverts commit 1c453d8.
… dead classes after the unification of reply button" This reverts commit 83c0ee2.
…eply" This reverts commit 2f96173.
…Options -> CommentActionsOptions also, delete unnecessary imports / components of Kotti-registered components" This reverts commit fe9874a.
This reverts commit 0199166.
This reverts commit 1e85813.
This reverts commit 802472e.
…comment-wrapper had flex: 1 1 auto changing it to flex: 1 means that it became flex: 1 1 0 the flex-basis is the third property/value changing it to `0` means that the elements WON'T change size after the space is redistributed this guarantees that the comment_wrapper doesn't take more space, ...and therefore squash the avatar" This reverts commit bf9d4cb.
32ff1fc
to
56080c1
Compare
nextPage: () => { | ||
if (currentPage.value === maximumPage.value) return | ||
currentPage.value += 1 | ||
emit('nextPageClicked', currentPage.value + 1) |
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.
don't know if you see the bug (already there before ur refactor :D)
but
you first update the currentPage.value then you emit currentPage + 1
if currentPage was 0 before this function got clicked
you'd emit 2
I suggested line 68 becomes
emit('nextPageClicked', currentPage.value)
or you make it ridiculously verbose
const nextPage = currentPage.value+1
currentPage.value = nextPage
emit('nextPageClicked', nextPage)
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.
ok now that i've seen the entire code, my guess is: currentPage is zero-based and page is one-based and we were circumventing this internally like this
honestly, would rather this whole issue is solved differently
instead of this weirdness, might as well start currentPage with this
also I'd say we can make page
prop in types.ts use zod.number().min(1)
this will error if user app passes anything less than 1
previousPage: () => { | ||
if (currentPage.value === 0) return | ||
currentPage.value -= 1 | ||
emit('previousPageClicked', currentPage.value + 1) |
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.
so the bug here is that we counteract the -1
so we are emitting the old page
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.
we seem to counteract it in some places in our user app
some usage example: @previousPageClicked="setPage(pageIndex-1)"
}, | ||
paginatorClasses: (page) => ({ | ||
'kt-pagination__page-item': true, | ||
'kt-pagination__page-item--is-disabled': currentPage.value === page, |
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.
if you fix the logic for the currentPage being zero-based while the page prop is one-based
don't forget to change this as well
(you'd have to pass 1
and maximumPage+1
which would be pageAmount
)
currentPage.value -= 1 | ||
emit('previousPageClicked', currentPage.value + 1) | ||
}, | ||
setCurrentPage: (page) => { |
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.
if you change currentPage to be one-paged, don't forget this argument as well (would have to be 1-based
db0f2aa
to
78dbab4
Compare
Closed, replaced by #1007 |
The usual shtick, some typescript, some zod, some rearranging of code and whatnot