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
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
334ffd0
refact(kottiPagination): typescript and composition api for parent co…
Isokaeder Oct 14, 2021
e4e2db7
refactor(kottiPagination): add prop types, zod schema and add to expo…
Isokaeder Oct 14, 2021
4f94b34
refact(kottiPagination): brush up css
Isokaeder Oct 14, 2021
626eb99
refact(kottiPagination): remove deprecated prop
Isokaeder Oct 14, 2021
4c2314c
docs(kottiPagination): remove prop table
Isokaeder Oct 14, 2021
4f03cdb
refact(kottiPagination): 'kt-pagination__' prefix for all css classes
Isokaeder Oct 14, 2021
1420772
refactor(kottiPagination): clean up some functions
Isokaeder Oct 14, 2021
ce9772c
fix(kottiPagination): put css variable into root
Isokaeder Oct 14, 2021
91598cb
refact(kotti-pagination): inline function that calculates center of s…
Isokaeder Oct 14, 2021
56080c1
fix(KtPagination): emit right value
Isokaeder Dec 1, 2021
bf9d4cb
fix(KtComment): avatar gets squashed with large comments
carsoli May 3, 2022
802472e
refact(KtComment): move classes around w/o changes
carsoli May 12, 2022
1e85813
feat(KtComment): add translations to edit and delete
carsoli May 13, 2022
0199166
feat(KtComment): add CommentOptions component
carsoli May 13, 2022
fe9874a
fix(CommentActionsOptions): clickbehavior prop
carsoli May 13, 2022
2f96173
feat(KtComment): implement CommentActions and CommentActionsReply
carsoli May 13, 2022
83c0ee2
refact(CommentReply): move classes from _comments.scss
carsoli May 13, 2022
1c453d8
feat(KtCommentInput): prefix classes with `kt-`
carsoli May 13, 2022
e144bc5
feat(KtComment): prefix classes with `kt-`
carsoli May 13, 2022
ee74729
fix(KtComment): fix editing of replies
carsoli May 18, 2022
134d42b
refact(KtComment): use CommentInlineEdit
carsoli May 18, 2022
9d09175
fix(CommentReply): fix props passed to it and add types
carsoli May 18, 2022
3f1bb38
fix(CommentInlineEdit): fix font-size and remove dead css
carsoli May 18, 2022
eaab11e
refact(KtComment): unified edit event - similar to delete
carsoli May 23, 2022
0c29297
fix(KtPopover): not sure why we even need min width
carsoli May 23, 2022
0ed4bab
enhance(CommentInlineEdit): scrollbar styles
carsoli May 24, 2022
1dd00e5
feat(KtComment): parse \n by default in post parser
carsoli May 24, 2022
6a9114c
chore(stylelint): fix
carsoli May 24, 2022
c5bcff2
refact(CommentReply) use css variables
carsoli May 24, 2022
9ca48f6
Revert "refact(CommentReply) use css variables fix translations as well"
carsoli May 25, 2022
a2e0482
Revert "chore(stylelint): fix"
carsoli May 25, 2022
a819a6b
Revert "feat(KtComment): parse \n by default in post parser"
carsoli May 25, 2022
9b3f631
Revert "enhance(CommentInlineEdit): scrollbar styles"
carsoli May 25, 2022
01c63f5
Revert "fix(KtPopover): not sure why we even need min width"
carsoli May 25, 2022
df90dc9
Revert "refact(KtComment): unified edit event - similar to delete"
carsoli May 25, 2022
4fb1a21
Revert "fix(CommentInlineEdit): fix font-size and remove dead css"
carsoli May 25, 2022
b371b32
Revert "fix(CommentReply): fix props passed to it and add types"
carsoli May 25, 2022
6340853
Revert "refact(KtComment): use CommentInlineEdit"
carsoli May 25, 2022
ffcaf5e
Revert "fix(KtComment): fix editing of replies previously, editing wa…
carsoli May 25, 2022
6f0097b
Revert "feat(KtComment): prefix classes with `kt-`"
carsoli May 25, 2022
f030c79
Revert "feat(KtCommentInput): prefix classes with `kt-` move styles t…
carsoli May 25, 2022
9e68054
Revert "refact(CommentReply): move classes from _comments.scss remove…
carsoli May 25, 2022
65ed5e2
Revert "feat(KtComment): implement CommentActions and CommentActionsR…
carsoli May 25, 2022
d9c02ae
Revert "fix(CommentActionsOptions): clickbehavior prop rename Comment…
carsoli May 25, 2022
a317690
Revert "feat(KtComment): add CommentOptions component"
carsoli May 25, 2022
f7b4e64
Revert "feat(KtComment): add translations to edit and delete"
carsoli May 25, 2022
5561c18
Revert "refact(KtComment): move classes around w/o changes"
carsoli May 25, 2022
8016a80
Revert "fix(KtComment): avatar gets squashed with large comments the …
carsoli May 25, 2022
0e56ed5
refact(KottiPagination): typescript and composition api for child com…
Isokaeder Oct 14, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions packages/documentation/pages/usage/components/pagination.vue
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,6 @@

## Usage

### Props

| Attribute | Description | Type | Accepted Values | Default |
| :--------------- | :------------------------------------------- | :-------- | :--------------------------- | :--------- |
| `adjacentAmount` | number of pairs of adjacent pages to display | `Number` | -- | `1` |
| `fixedWidth` | set width based on max number of elements | `Boolean` | `True`, `False` | `False` |
| `page` | the default page to show | `Number` | -- | -- |
| `pageSize` | amount of items each page | `Number` | -- | `10` |
| `pagingStyle` | style of the pagination | `String` | `expand`, `flex`, `fraction` | `expand` |
| `total` | total amount of items | `Number` | -- | `Required` |

### Events

| Event Name | Description | Parameters |
Expand Down
200 changes: 88 additions & 112 deletions packages/kotti-ui/source/kotti-pagination/KtPagination.vue
Original file line number Diff line number Diff line change
@@ -1,163 +1,139 @@
<template>
<div>
<ul class="pagination">
<li :class="paginatorClasses(0, 'disabled')" @click="previousPage">
<i class="yoco page-button">chevron_left</i>
<ul class="kt-pagination">
<li :class="paginatorClasses(0)" @click="previousPage">
<i
class="yoco kt-pagination__page-button"
v-text="Yoco.Icon.CHEVRON_LEFT"
/>
</li>
<component
:is="component"
v-bind="bound"
:is="paginationComponent"
v-bind="paginationProps"
@setPage="setCurrentPage($event)"
/>
<li :class="paginatorClasses(maximumPage, 'disabled')" @click="nextPage">
<i class="yoco page-button">chevron_right</i>
<li :class="paginatorClasses(maximumPage)" @click="nextPage">
<i
class="yoco kt-pagination__page-button"
v-text="Yoco.Icon.CHEVRON_RIGHT"
/>
</li>
</ul>
</div>
</template>

<script>
<script lang="ts">
import { Yoco } from '@3yourmind/yoco'
import { computed, defineComponent, ref } from '@vue/composition-api'

import { makeProps } from '../make-props'

import PaginationExpanded from './components/PaginationExpanded.vue'
import PaginationFlexible from './components/PaginationFlexible.vue'
import PaginationFractionated from './components/PaginationFractionated.vue'
import { KottiPagination } from './types'

export default {
export default defineComponent<KottiPagination.PropsInternal>({
name: 'KtPagination',
components: {
PaginationExpanded,
PaginationFlexible,
PaginationFractionated,
},
props: {
adjacentAmount: { type: Number, default: 1 },
fixedWidth: { type: Boolean, default: false },
page: { type: Number, default: 1 },
pageSize: { type: Number, default: 10 },
pagingStyle: { type: String, default: 'expand' },
total: { type: Number, required: true },
props: makeProps(KottiPagination.propsSchema),
setup(props, { emit }) {
const currentPage = ref(props.page - 1)
Copy link
Member

Choose a reason for hiding this comment

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

This API should probably just be controlled so we can ensure that the page is properly handled.

I’d suggest either a follow-up PR after this one is merged, or an issue if you don’t want to do this immediately

const pageAmount = computed(() => Math.ceil(props.total / props.pageSize))
const maximumPage = computed(() => pageAmount.value - 1)

/**
* @deprecated
* Use :pagingStyle='fraction' instead
*/
fractionStyle: { type: Boolean, default: false },
},
data() {
return {
currentPage: this.page - 1,
paginationComponent: computed(() => {
Isokaeder marked this conversation as resolved.
Show resolved Hide resolved
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

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)

? PaginationExpanded.name
: PaginationFlexible.name
case 'fraction':
return PaginationFractionated.name
default:
return PaginationExpanded.name
}
}),
currentPage,
maximumPage,
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

},
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)

}),
paginationProps: computed(() => ({
adjacentAmount: props.adjacentAmount,
currentPage: currentPage.value,
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

})),
previousPage: () => {
if (currentPage.value === 0) return
currentPage.value -= 1
emit('previousPageClicked', currentPage.value + 1)
Copy link
Member

Choose a reason for hiding this comment

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

This API should probably be simplified. Why would the app care if the previous button was clicked instead of a specific page number? Let’s just emit the normal event

Maybe in a follow-up PR after this one is merged, alternatively let’s make an issue if you don’t want to do this immediately

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)"

},
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

if (page === currentPage.value) return
currentPage.value = page
emit('currentPageChange', currentPage.value + 1)
Copy link
Member

Choose a reason for hiding this comment

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

This API should probably be simplified. Why would the app care if the previous button was clicked instead of a specific page number? Let’s just emit the normal event

Maybe in a follow-up PR after this one is merged, alternatively let’s make an issue if you don’t want to do this immediately

},
Yoco,
}
},
computed: {
bound() {
return {
adjacentAmount: this.adjacentAmount,
currentPage: this.currentPage,
fixedWidth: this.fixedWidth,
maximumPage: this.maximumPage,
pageSize: this.pageSize,
total: this.total,
totalPages: this.totalPages,
}
},
component() {
const isFlexLogical = 2 * (this.adjacentAmount + 1) < this.pageAmount
if (!isFlexLogical || this.pageAmount < 2) return 'PaginationExpanded'

if (this.fractionStyle) {
// eslint-disable-next-line no-console
console.warn(
"<KtPagination>: fractionStyle is deprecated, please use :pagingStyle='fraction' instead",
)
return PaginationFractionated.name
}

switch (this.pagingStyle) {
case 'flex':
return PaginationFlexible.name
case 'fraction':
return PaginationFractionated.name
default:
return PaginationExpanded.name
}
},
maximumPage() {
return Math.ceil(this.total / this.pageSize) - 1
},
pageAmount() {
return Math.ceil(this.total / this.pageSize)
},
},
methods: {
paginatorClasses(page, className) {
return {
'page-item': true,
[className]: this.currentPage === page,
}
},
setCurrentPage(page) {
if (page === this.currentPage) return
this.currentPage = page
this.eventEmitter('currentPageChange')
},
nextPage() {
if (this.currentPage === this.maximumPage) return
this.currentPage += 1
this.eventEmitter('nextPageClicked')
},
previousPage() {
if (this.currentPage === 0) return
this.currentPage -= 1
this.eventEmitter('previousPageClicked')
},
eventEmitter(eventName) {
this.$emit(eventName, this.currentPage + 1)
},
},
}
})
</script>

<style lang="scss">
@import '../kotti-style/_variables.scss';

:root {
--pagination-color-active: var(--interactive-03);
--kt-pagination-color-active: var(--interactive-03);
}
.pagination {
</style>

<style lang="scss" scoped>
.kt-pagination {
margin: 0;
list-style: none;
user-select: none;
.page-button {
&__page-button {
display: inline-block;
padding: var(--unit-1);
background: $lightgray-300;
background: var(--gray-10);
border-radius: var(--border-radius);
&:hover {
cursor: pointer;
background: $lightgray-400;
background: var(--gray-20);
}
}
.fraction {
display: inline-block;
}
.page-item {
::v-deep &__page-item {
display: inline-block;
padding: var(--unit-2);
line-height: 24px;
text-align: center;
&--active {
color: var(--pagination-color-active);
&--is-active {
color: var(--kt-pagination-color-active);
}
&--is-disabled {
cursor: not-allowed;

.kt-pagination__page-button {
opacity: 0.46;
}
}
&:hover {
cursor: pointer;
}
}

.disabled {
cursor: not-allowed;

.page-button {
opacity: 0.46;
}
}
}
</style>
Original file line number Diff line number Diff line change
@@ -1,47 +1,46 @@
<template>
<div class="inline-container">
<div class="kt-pagination__inline-container">
<li
v-for="(page, index) in totalPages"
:key="index"
:class="paginatorClasses(page, 'page-item--active')"
:class="paginatorClasses(page)"
@click="$emit('setPage', page)"
v-text="humanReadablePageNumber(page)"
/>
</div>
</template>

<script>
export default {
<script lang="ts">
import { computed, defineComponent } from '@vue/composition-api'
import range from 'lodash/range'

export default defineComponent<{
currentPage: number
pageSize: number
total: number
}>({
name: 'PaginationExpanded',
props: {
currentPage: { type: Number, required: true },
pageSize: { type: Number, required: true },
total: { type: Number, required: true },
},
computed: {
pageAmount() {
return Math.ceil(this.total / this.pageSize)
},
totalPages() {
return [...Array(this.pageAmount).keys()]
},
},
methods: {
humanReadablePageNumber(page) {
return page + 1
},
paginatorClasses(page, className) {
return {
'page-item': true,
[className]: this.currentPage === page,
}
},
setup(props) {
const pageAmount = computed(() => Math.ceil(props.total / props.pageSize))
return {
humanReadablePageNumber: (page: number) => page + 1,
paginatorClasses: (page: number) => ({
'kt-pagination__page-item': true,
'kt-pagination__page-item--is-active': props.currentPage === page,
}),
totalPages: computed(() => range(pageAmount.value)),
}
},
}
})
</script>

<style lang="scss" scoped>
.inline-container {
.kt-pagination__inline-container {
display: inline;
}
</style>
Loading