Skip to content

fix: keep DataTable container filled when paginate #6439

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Nov 1, 2022
Merged

fix: keep DataTable container filled when paginate #6439

merged 9 commits into from
Nov 1, 2022

Conversation

matheushahnn
Copy link
Contributor

What does this PR do?

It will keep the fill style when DataTable has pagination

Where should the reviewer start?

What testing has been done on this PR?

Manual test with other storybooks and one unit test

How should this be manually tested?

Copy the component from https://codesandbox.io/s/grommet-v2-not-working-pin-with-paginate-ctth91

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

#6333

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

jcfilben
jcfilben reviewed Oct 20, 2022
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

I tested with the following code and I am still seeing the issue

<Box margin="large" height="200px" overflow="auto">
  <DataTable
    pin="header"
    columns={columns}
    data={DATA}
    step={5}
    paginate
  />
</Box>

@jcfilben jcfilben added the waiting Awaiting response to latest comments label Oct 24, 2022
@matheushahnn
Copy link
Contributor Author

You're right, thank you for showing me that issue.
Sorry about the delay in responding.

There is a comment about not enabling flex on

grommet/src/js/components/DataTable/DataTable.js

Line 355 in 83d9e9c

? { overflow: { horizontal: 'auto' }, flex: false, fill }

but if I set as true it will work, so this is one option to keep the header sticky
image

I am investigating another way to consider pagination height in the table content to keep pagination inside scroll.

@matheushahnn
Copy link
Contributor Author

Now the pagination is at the bottom
image

lmk if it is ok, or there is a case that it breaks, please

@jcfilben jcfilben removed the waiting Awaiting response to latest comments label Oct 25, 2022
@jcfilben
Copy link
Collaborator

lmk if it is ok, or there is a case that it breaks, please

I think moving the pagination controls into the overflow container may not be the best user experience. I would expect to be able to switch pages without having to scroll through the table. Also if pin=true and both the header and footer of the table are pinned it seems really odd that you still have to scroll through the table to get to the pagination controls.

It looks like setting flex: true is working well. I think if we just do that and leave the paginated controls outside of the overflow container that could work.

jcfilben
jcfilben approved these changes Oct 25, 2022
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

This looks good to me, I'm going to tag @taysea as well for review on this. I just want to make sure that there are no issues with setting flex: true that I may have missed.

@jcfilben jcfilben requested a review from taysea October 25, 2022 19:56
taysea
taysea approved these changes Oct 28, 2022
View reviewed changes
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

Behavior looks good to me, I'm trying to remember why we'd initially decided to do flex: false with Pagination PR.

@ericsoderberghp ericsoderberghp merged commit 63aa106 into grommet:master Nov 1, 2022
@jcfilben jcfilben mentioned this pull request Nov 2, 2022
Closed
@jcfilben jcfilben mentioned this pull request Nov 16, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taysea taysea taysea approved these changes

@jcfilben jcfilben jcfilben approved these changes

+1 more reviewer

@ericsoderberghp ericsoderberghp ericsoderberghp approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matheushahnn @jcfilben @ericsoderberghp @taysea