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

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

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 16 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
fixedWidth: props.fixedWidth,
maximumPage: maximumPage.value,
pageSize: props.pageSize,
total: props.total,
Copy link
Contributor

Choose a reason for hiding this comment

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

removed totalPages from object

added adjacentAmount

paginationComponent: computed(() => {
const isFlexLogical = 2 * (props.adjacentAmount + 1) < pageAmount.value
switch (props.pagingStyle) {
case 'flex':
Copy link
Contributor

Choose a reason for hiding this comment

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

why not

PagingStyle.FLEX

& for the next case PagingStyle.FRACTION

const isFlexLogical = 2 * (props.adjacentAmount + 1) < pageAmount.value
switch (props.pagingStyle) {
case 'flex':
return !isFlexLogical || pageAmount.value < 2
Copy link
Contributor

Choose a reason for hiding this comment

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

you want to fallback to the default case if a certain condition is not met

so IMO

case 'flex': { if(isFlexLogical && pageAmount.value >=2) return PaginationFlexible.name }
case 'fraction': return PaginationFractionated.name
case 'expanded': 
default: return PaginationExpanded.name

Copy link
Contributor

Choose a reason for hiding this comment

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

returning always breaks out of the switch

but since we don't return, the switch will continue to the next plausible cases (not going to match fraction/expanded but will match default)

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

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.

None yet

3 participants