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

Refactor kotti pagination #543

Closed
wants to merge 49 commits into from
Closed

Conversation

Isokaeder
Copy link
Collaborator

The usual shtick, some typescript, some zod, some rearranging of code and whatnot

@render
Copy link

render bot commented Oct 14, 2021

@FlorianWendelborn FlorianWendelborn added autorebase:opt-in Apply this label to enable automatic rebasing package:kotti-ui @3yourmind/kotti-ui priority:3-normal Should be fixed soon type:refactor labels Oct 14, 2021
@FlorianWendelborn FlorianWendelborn force-pushed the refactor-kotti-pagination branch 3 times, most recently from 3e8d3bd to 4deb70d Compare November 22, 2021 22:29
@FlorianWendelborn FlorianWendelborn force-pushed the refactor-kotti-pagination branch 5 times, most recently from b1642f2 to 20f996e Compare November 24, 2021 15:10
carsoli and others added 14 commits May 25, 2022 11:44
…s internally mutating the replies"

This reverts commit ee74729.
…o component (from _comments.scss)"

This reverts commit 1c453d8.
… dead classes after the unification of reply button"

This reverts commit 83c0ee2.
…Options -> CommentActionsOptions also, delete unnecessary imports / components of Kotti-registered components"

This reverts commit fe9874a.
…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.
@FlorianWendelborn FlorianWendelborn force-pushed the refactor-kotti-pagination branch 2 times, most recently from 32ff1fc to 56080c1 Compare May 25, 2022 09:45
@FlorianWendelborn FlorianWendelborn added the autorebase:non-rebaseable AutoRebase applies this label when a pull request can't be rebased automatically label May 25, 2022
nextPage: () => {
if (currentPage.value === maximumPage.value) return
currentPage.value += 1
emit('nextPageClicked', currentPage.value + 1)
Copy link
Contributor

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)

Copy link
Contributor

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)
Copy link
Contributor

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

Copy link
Contributor

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,
Copy link
Contributor

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) => {
Copy link
Contributor

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

@Isokaeder
Copy link
Collaborator Author

Closed, replaced by #1007

@Isokaeder Isokaeder closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autorebase:non-rebaseable AutoRebase applies this label when a pull request can't be rebased automatically autorebase:opt-in Apply this label to enable automatic rebasing package:kotti-ui @3yourmind/kotti-ui priority:3-normal Should be fixed soon type:refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants