Copilot commented on code in PR #516: URL: https://github.com/apache/skywalking-booster-ui/pull/516#discussion_r2664389302
########## src/views/components/Pagination.vue: ########## @@ -0,0 +1,100 @@ +<!-- Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. --> +<template> + <div class="pagination-container"> + <el-pagination + v-model:current-page="currentPageModel" + :page-size="displayPageSize" + size="small" + layout="prev, pager, next" + :total="computedTotal" + :pager-count="pagerCount" + @current-change="handlePageChange" + :style="paginationStyle" + /> + </div> +</template> +<script lang="ts" setup> + import { computed } from "vue"; + + /*global defineProps, defineEmits*/ + const props = defineProps({ + currentPage: { + type: Number, + default: 1, + }, + pageSize: { + type: Number, + required: true, + }, + total: { + type: Number, + required: true, + }, + pagerCount: { + type: Number, + default: 5, + }, + align: { + type: String as () => "left" | "center" | "right", + default: "right", + }, + }); + + const emits = defineEmits<{ + (e: "update:currentPage", page: number): void; + (e: "change", page: number): void; + }>(); + + // The display page size is pageSize - 1 because we fetch pageSize items + // but the last item is only used to check if there are more pages + const displayPageSize = computed(() => props.pageSize - 1); + + // Calculate total for pagination display based on fetched items + // If we fetched pageSize items, there might be more pages + const computedTotal = computed(() => { + if (props.total >= props.pageSize) { + return displayPageSize.value * props.currentPage + 1; + } + return displayPageSize.value * props.currentPage; + }); + + const currentPageModel = computed({ + get: () => props.currentPage, + set: (val: number) => emits("update:currentPage", val), + }); + + const paginationStyle = computed(() => { + const alignMap = { + left: "flex-start", + center: "center", + right: "flex-end", + }; + return { + display: "flex", + justifyContent: alignMap[props.align], + }; + }); + + function handlePageChange(page: number) { + emits("update:currentPage", page); + emits("change", page); + } Review Comment: The update:currentPage event is being emitted twice when a page changes. The currentPageModel setter (line 76) emits it once, and handlePageChange (line 92) emits it again. Since both v-model:current-page and @current-change are bound to the el-pagination component, when a user clicks a page button, both the v-model mechanism and the current-change event handler fire, causing duplicate emissions. Remove the emit from the currentPageModel setter and let only handlePageChange emit both events, or remove the @current-change handler entirely and move the change event emission to the setter. ########## src/views/components/Pagination.vue: ########## @@ -0,0 +1,100 @@ +<!-- Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. --> +<template> + <div class="pagination-container"> + <el-pagination + v-model:current-page="currentPageModel" + :page-size="displayPageSize" + size="small" + layout="prev, pager, next" + :total="computedTotal" + :pager-count="pagerCount" + @current-change="handlePageChange" + :style="paginationStyle" + /> + </div> +</template> +<script lang="ts" setup> + import { computed } from "vue"; + + /*global defineProps, defineEmits*/ + const props = defineProps({ + currentPage: { + type: Number, + default: 1, + }, + pageSize: { + type: Number, + required: true, + }, + total: { + type: Number, + required: true, + }, Review Comment: The pagination logic implements a non-standard pattern where the component receives only the current page's data length (not total items across all pages). While this works for the current use case, this behavior should be documented in a comment at the component level to help future developers understand why 'total' represents current page items rather than the conventional meaning of total items across all pages. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
