pierrejeambrun commented on code in PR #65388: URL: https://github.com/apache/airflow/pull/65388#discussion_r3112232851
########## airflow-core/src/airflow/ui/src/layouts/Details/Grid/GridPaginationButtons.tsx: ########## @@ -0,0 +1,88 @@ +/*! + * 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. + */ +import { Flex, IconButton } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { FiChevronLeft, FiChevronRight, FiChevronsRight } from "react-icons/fi"; + +type Props = { + readonly hasNewerRuns: boolean; + readonly hasOlderRuns: boolean; + readonly isLoading: boolean; + readonly latestNotVisible: boolean; + readonly onJumpToLatest: () => void; + readonly onNewerRuns: () => void; + readonly onOlderRuns: () => void; +}; + +/** + * Absolutely-positioned navigation buttons that appear at the left and right + * edges of the grid header bar area. They do not participate in the flex + * layout so they never affect column alignment. + */ +export const GridPaginationButtons = ({ + hasNewerRuns, + hasOlderRuns, + isLoading, + latestNotVisible, + onJumpToLatest, + onNewerRuns, + onOlderRuns, +}: Props) => { + const { t: translate } = useTranslation("dag"); + + // Shared props applied to every pagination button. + const buttonProps = { loading: isLoading, size: "2xs", variant: "ghost" } as const; + + const newerLabel = translate("grid.buttons.newerRuns"); + const resetLabel = translate("grid.buttons.resetToLatest"); + const olderLabel = translate("grid.buttons.olderRuns"); + + return ( + <> + {latestNotVisible || hasNewerRuns ? ( + <Flex bottom={0} flexDirection="column" gap={1} position="absolute" right={-6} zIndex={1}> + {hasNewerRuns ? ( + <IconButton aria-label={newerLabel} onClick={onNewerRuns} title={newerLabel} {...buttonProps}> + <FiChevronRight /> + </IconButton> + ) : undefined} + {latestNotVisible ? ( + <IconButton aria-label={resetLabel} onClick={onJumpToLatest} title={resetLabel} {...buttonProps}> + <FiChevronsRight /> + </IconButton> + ) : undefined} + </Flex> + ) : undefined} + {hasOlderRuns ? ( + <IconButton + aria-label={olderLabel} + bottom={0} + left={-6} + onClick={onOlderRuns} + position="absolute" + title={olderLabel} + zIndex={1} + {...buttonProps} + > Review Comment: I would place `onNewerRuns` and `onOlderRuns` at the same vertical level (on the same 'row' basically) and the `onJumpToLatest` on a different one. For now `onJumpToLatest` and `onOlderRuns` are on the same level which feels weird, for a while I thought there were no `onNewerRuns` button. ########## airflow-core/src/airflow/ui/src/layouts/Details/Grid/GridPaginationButtons.tsx: ########## @@ -0,0 +1,88 @@ +/*! + * 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. + */ +import { Flex, IconButton } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { FiChevronLeft, FiChevronRight, FiChevronsRight } from "react-icons/fi"; + +type Props = { + readonly hasNewerRuns: boolean; + readonly hasOlderRuns: boolean; + readonly isLoading: boolean; + readonly latestNotVisible: boolean; + readonly onJumpToLatest: () => void; + readonly onNewerRuns: () => void; + readonly onOlderRuns: () => void; +}; + +/** + * Absolutely-positioned navigation buttons that appear at the left and right + * edges of the grid header bar area. They do not participate in the flex + * layout so they never affect column alignment. + */ +export const GridPaginationButtons = ({ + hasNewerRuns, + hasOlderRuns, + isLoading, + latestNotVisible, + onJumpToLatest, + onNewerRuns, + onOlderRuns, +}: Props) => { + const { t: translate } = useTranslation("dag"); + + // Shared props applied to every pagination button. + const buttonProps = { loading: isLoading, size: "2xs", variant: "ghost" } as const; + + const newerLabel = translate("grid.buttons.newerRuns"); + const resetLabel = translate("grid.buttons.resetToLatest"); + const olderLabel = translate("grid.buttons.olderRuns"); + + return ( + <> + {latestNotVisible || hasNewerRuns ? ( + <Flex bottom={0} flexDirection="column" gap={1} position="absolute" right={-6} zIndex={1}> + {hasNewerRuns ? ( + <IconButton aria-label={newerLabel} onClick={onNewerRuns} title={newerLabel} {...buttonProps}> + <FiChevronRight /> + </IconButton> + ) : undefined} + {latestNotVisible ? ( + <IconButton aria-label={resetLabel} onClick={onJumpToLatest} title={resetLabel} {...buttonProps}> + <FiChevronsRight /> + </IconButton> + ) : undefined} + </Flex> + ) : undefined} + {hasOlderRuns ? ( + <IconButton + aria-label={olderLabel} + bottom={0} + left={-6} + onClick={onOlderRuns} + position="absolute" + title={olderLabel} + zIndex={1} + {...buttonProps} + > Review Comment: <img width="655" height="310" alt="Image" src="https://github.com/user-attachments/assets/6fb59d70-51f7-4ab3-b93f-dbf621698277"; /> ########## airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridPagination.ts: ########## @@ -0,0 +1,77 @@ +/*! + * 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. + */ +import type { GridRunsResponse } from "openapi/requests"; +import { useGridRuns } from "src/queries/useGridRuns.ts"; + +type Params = { + gridRuns: Array<GridRunsResponse> | undefined; + limit: number; + offset: number; + setOffset: (value: number) => void; +}; + +export type PaginationState = { + hasNewerRuns: boolean; + hasOlderRuns: boolean; + latestNotVisible: boolean; +}; + +type Result = { + handleNewerRuns: () => void; + handleOlderRuns: () => void; +} & PaginationState; + +/** + * Pure helper — derives pagination flags from already-fetched data. + * Kept separate so it can be tested without React or network dependencies. + */ +export const computePaginationState = ({ + gridRuns, + latestRunId, + limit, + offset, +}: { + gridRuns: Array<GridRunsResponse> | undefined; + latestRunId: string | undefined; + limit: number; + offset: number; +}): PaginationState => ({ + hasNewerRuns: offset > 0, + hasOlderRuns: (gridRuns?.length ?? 0) >= limit, Review Comment: I think that this can be inaccurate and I've hit this case. If we are perfectly on 'limit' and offset '0', we can end up on an empty page. We should probably fetch `limit + 1` and trim in front-end to get an accurate `has more` signal -- 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]
