Skip to content

feat(Pagination): add possibility to use wrapper components for pagination buttons #2040

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Arucard89
Copy link
Contributor

@Arucard89 Arucard89 commented Jan 14, 2025

UXRFC-439

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

1 similar comment
@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

@Arucard89 Arucard89 changed the title feat: add possibility to use pagination buttons as links feat(Pagination): add possibility to use pagination buttons as links Jan 14, 2025
@Arucard89 Arucard89 marked this pull request as ready for review January 14, 2025 10:45
@Arucard89 Arucard89 requested a review from jhoncool as a code owner January 14, 2025 10:45
return;
}

const newPage = page > numberOfPages ? numberOfPages : page;

onUpdate(newPage, newPageSize);
if (pageHrefBuilder) {
window.location.href = pageHrefBuilder(newPage, newPageSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually you want to work with react-router and SPA links. So reloading the page it's a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that the main goal of the ticket was to use links (<a/> tag) in all controls. And click for them will also reload page. So we have the same behaviour in all components.
I suppose, if user want to use links in Pagination he know all the pros and cons of this case.

You still also able to use onUpdate prop for react-router way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to support both cases. When there are links <a> and you can use react-router also. So we need to prevent default click but also we can open new tab on ctrl/cmd+click

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be better to try to go another way. I'll make prop to pass custom wrapper for Pagination controls. And user will decide what it will be. It could be common <a/>, <Link/> or <NavLink/> from react-router or anything else.
In my opinion, it will be more flexible solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to try to go another way. I'll make prop to pass custom wrapper for Pagination controls. And user will decide what it will be. It could be common <a/>, <Link/> or <NavLink/> from react-router or anything else. In my opinion, it will be more flexible solution.

Sounds good

@Arucard89
Copy link
Contributor Author

I rewrote component to support different ways of usage
@jhoncool, could you review it again?

| className | HTML-атрибут `class`. | `string` | |
| compact | Скрывает заголовки для кнопок `First`, `Previous` и `Next`. В мобильной версии всегда имеет значение `true`. | `boolean` | `true` |
| buttonWrapper | Компонент-обертка над кнопками пагинации | `Function` | |
| onUpdate | Вызывается при изменении номера страницы или свойства `pageSize` (ели задан параметр `buttonWrapper`, то `onUpdate` вызывается только в `input` и `select`). | `Function` | |
Copy link
Contributor

Choose a reason for hiding this comment

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

typo ели

Copy link
Contributor Author

Choose a reason for hiding this comment

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

export type ButtonWrapperParam = {
page: number;
pageSize: number;
button: React.ReactElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set type for props here? Because you used button.props.disabled in your examples. It's any type now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -25,6 +25,7 @@ export const Pagination = ({
total,
size: propSize,
onUpdate,
buttonWrapper,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename it to paginationItemWrapper for example. Because it can be not only Button in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may be shorter itemWrapper will be better? also I renamed param name inside itemWrapper from button to item according to the new name of parent param

9d4b9af

@jhoncool
Copy link
Contributor

@amje @korvin89 will any of you please review it too?


При использовании этого способа пользователь будет взаимодействовать кнопками пагинации, как с произвольными компонентами.

Для этого необходимо указать свойство `buttonWraper`. В этом случае `onUpdate` будет вызываться только при изменении компонентов input и select.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Для этого необходимо указать свойство `buttonWraper`. В этом случае `onUpdate` будет вызываться только при изменении компонентов input и select.
Для этого необходимо указать свойство `itemWrapper`. В этом случае `onUpdate` будет вызываться только при изменении компонентов input и select.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


In this case user will iteract with pagination's controls as with buttons.

Set `onUpdate` prop to use pagination as buttons (`buttonWraper` must be `undefined`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Set `onUpdate` prop to use pagination as buttons (`buttonWraper` must be `undefined`).
Set `onUpdate` prop to use pagination as buttons (`itemWrapper` must be `undefined`).

Copy link
Contributor Author

Choose a reason for hiding this comment

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


In this case user will iteract with pagination's buttons as with custom controls.

Set `buttonWraper` prop to use pagination as custom contrlols. In this case `onUpdate` will fire only on input and select components.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Set `buttonWraper` prop to use pagination as custom contrlols. In this case `onUpdate` will fire only on input and select components.
Set `itemWrapper` prop to use pagination as custom controls. In this case `onUpdate` will fire only on input and select components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@korvin89
Copy link
Contributor

image
@Arucard89 this is invalid html, more details and specs here

@Arucard89
Copy link
Contributor Author

@korvin89 , I made correct nested element for wrapper

@korvin89
Copy link
Contributor

@korvin89 , I made correct nested element for wrapper

Still incorrect:
image

@Arucard89
Copy link
Contributor Author

Arucard89 commented Jan 24, 2025

@korvin89 , something wrong with preview. It has not been updated. Because in my local build everything is OK:

image

Is there any way to rebuild preview?

@Arucard89 Arucard89 changed the title feat(Pagination): add possibility to use pagination buttons as links feat(Pagination): add possibility use custom pagination button components Jan 24, 2025
@Arucard89 Arucard89 changed the title feat(Pagination): add possibility use custom pagination button components feat(Pagination): add possibility to use wrapper components for pagination buttons Jan 24, 2025
@korvin89
Copy link
Contributor

something wrong with preview

Yep! My bad, sorry

```jsx
import {Pagination, PaginationProps} from '@gravity-ui/uikit';

const [state, setState] = React.useState({page: 1, pageSize: 100});

const handleUpdate: PaginationProps['onUpdate'] = (page, pageSize) =>
setState((prevState) => ({...prevState, page, pageSize}));
setState((prevState) => ({page, pageSize}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use one style in the README file

setState((prevState) => ({...prevState, page, pageSize}));
// or
setState({page, pageSize}})

const [state, setState] = React.useState({page: 1, pageSize: 100});
const navigate = useNavigate();

const renderWrapper = ({page, pageSize, button}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

button -> item


In this case user will iteract with pagination's buttons as with custom controls.

Set `itemWrapper` prop to use pagination as custom contrlols. In this case `onUpdate` will fire only on input and select components.
Copy link
Contributor

Choose a reason for hiding this comment

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

contrlols -> controls

const [state, setState] = React.useState({page: 1, pageSize: 100});
const navigate = useNavigate();

const renderWrapper = ({page, pageSize, button}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

button -> item

@Arucard89
Copy link
Contributor Author

Arucard89 commented Jan 24, 2025

@jhoncool , I fixed typo and rewrote examples for readme: 6be0fdb

@Arucard89
Copy link
Contributor Author

Arucard89 commented Jan 27, 2025

@korvin89 , @amje , may I merge this PR into main?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants