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

DataLis pagination demo #10276

Open
wants to merge 5 commits into
base: v5
Choose a base branch
from

Conversation

ajaypratap003
Copy link
Member

@ajaypratap003 ajaypratap003 commented Apr 15, 2024

What: Closes #10247
DataList pagination demo

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Apr 15, 2024

@tlabaj tlabaj requested review from a team and mmenestr and removed request for a team April 17, 2024 13:58
Copy link
Contributor

@gitdallas gitdallas left a comment

Choose a reason for hiding this comment

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

let's get a basic pagination working, and more than 10 items to see it (created with a loop so all the code isn't just for the items themselves).

@ajaypratap003
Copy link
Member Author

ajaypratap003 commented Apr 19, 2024

Thanks you all for reviewing PR. In Github issue, it was about the static pagination. That's why I made it static pagination, not dynamic.
Anyway I made it dynamic and working as expected. I've implemented all the review comments. Please let me know if any review comment missing.

@gitdallas
Copy link
Contributor

i understand the confusion there, HTML demos are always static in that sense necessarily though.

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Couple more comments below, not really blockers though.

packages/react-table/src/demos/sampleData.tsx Outdated Show resolved Hide resolved
packages/react-core/src/demos/sampleData.tsx Outdated Show resolved Hide resolved
@ajaypratap003 ajaypratap003 changed the title DataList static pagination demo DataLis pagination demo Apr 25, 2024
@ajaypratap003
Copy link
Member Author

Hey folks,
Can you please merge this PR if everything looks fine

@andrew-ronaldson andrew-ronaldson self-requested a review May 13, 2024 12:56
Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

It looks like something went wrong with your rebase. You have files here that should not have been updated in your PR.

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

Left some comments, the only potential blocker is that the issue title and demo to match is core's static pagination demo which uses the static variation of bottom pagination (isStatic). This demo is sticky bottom pagination, but with full pagination support. If we were looking for parity with the core demos, maybe we would:

  • Update the basic demo to have full pagination support, and add compact bottom pagination to that demo.
  • Update this demo title to "Static bottom pagination" and remove isSticky and add isStatic to the bottom pagination

The other comments are just nice-to-haves

  • Match the flex layout(s) from core for the data list items. That will address some spacing differences between react/core
  • Use compact top pagination

<DataListItemCells
dataListCells={[
<DataListCell isFilled={false} key="buttons1">
<Flex direction={{ default: 'column' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a nit, I'm not sure how close we need to match the core demo, especially this part which is just content and doesn't impact the purpose of the demo (static bottom pagination) but if we do want to fix it, the layout is below. One decent visual thing it will address is to add space between the icon/text pairings in each row. You could also just add a {' '} or something there, too, or get the svg and number to render on the same line like the basic data list demo

flex.column.space-items-md
  flex.column.space-items-none
    flex-item
      // <p> text
    flex-item
      // <small> text
  flex.wrap
    flex.space-items-xs // one of these for each svg/number pair
      // <svg> and number
    flex-item
      // "Updated [n] days ago" text

@@ -28,3 +29,8 @@ import { DashboardWrapper } from '@patternfly/react-core/dist/js/demos/Dashboard

```js file="./DataList/examples/DataListExpandableControlInToolbar.tsx" isFullscreen
```

### Pagination
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is to create a static bottom pagination demo, I'd rename this. The core demo is called "Static bottom pagination"

</OverflowMenu>
</ToolbarItem>
<ToolbarItem variant="pagination" align={{ default: 'alignRight' }}>
{renderPagination('top-pagination', PaginationVariant.top, false, false)}
Copy link
Contributor

Choose a reason for hiding this comment

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

To match core (and the basic react data-list demo) this would use compact top pagination

Suggested change
{renderPagination('top-pagination', PaginationVariant.top, false, false)}
{renderPagination('top-pagination', PaginationVariant.top, true, false)}

);
})}
</DataList>
{renderPagination('bottom-pagination', PaginationVariant.bottom, true, true)}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the issue title in #10247, if this is supposed to match the HTML/core static bottom pagination demo, you'll want to pass isStatic to this bottom pagination. By default bottom pagination is "sticky" on mobile, meaning it floats on the bottom of the viewport as you scroll. A static pagination won't float on mobile, and will always be at the very end of the datalist. You'll want to make this change, disabling isSticky

Suggested change
{renderPagination('bottom-pagination', PaginationVariant.bottom, true, true)}
{renderPagination('bottom-pagination', PaginationVariant.bottom, true, false)}

Then I'm guessing you might need another arg in renderPagination() to set isStatic

const renderPagination = (id: string, variant: PaginationVariant, isCompact: boolean, isSticky: boolean) => (
<Pagination
id={id}
variant={variant}
itemCount={rows.length}
page={page}
perPage={perPage}
isCompact={isCompact}
isSticky={isSticky}
onSetPage={handleSetPage}
onPerPageSelect={handlePerPageSelect}
titles={{
paginationAriaLabel: `${variant} pagination`
}}
/>
);

Copy link

github-actions bot commented Aug 6, 2024

This PR has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale label Aug 6, 2024
@github-actions github-actions bot removed the stale label Sep 5, 2024
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.

DataList demo with Static bottom pagination
8 participants