Skip to content

Conversation

@jenny-s51
Copy link
Contributor

What: Closes #7309

@patternfly-build
Copy link
Collaborator

patternfly-build commented May 2, 2022

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

LGTM just needs a rebase

@nicolethoen nicolethoen requested review from mcarrano and mcoker May 5, 2022 00:50
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

I notice that the core demo includes bottom pagination. I wonder if we'd usually recommend that a table with pagination in the toolbar also includes it at the bottom, @mcarrano?

@mcarrano
Copy link
Member

mcarrano commented May 5, 2022

I wonder if we'd usually recommend that a table with pagination in the toolbar also includes it at the bottom, @mcarrano?

Generally yes. It gives the user the ability to click to the next page from either place without needing to scroll the page.

Copy link
Member

@mcarrano mcarrano 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 @jenny-s51 !

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.

This is more of a general comment and not a blocker. The pagination controls do not match what is in the page. The same is true for core.
@mcarrano does this matter?

@mcarrano
Copy link
Member

mcarrano commented May 6, 2022

@tlabaj Yeah, I noticed that also when I looked at this yesterday and decided to not comment on it. Ideally I think we should have a standard table/data set that we use for all these demos that contains a good number of data records (like 50-100?) and with proper working pagination. Then that could be used as a common starting point for all demos on a table. If you agree, should we open an issue for this? Maybe we need to create some sample data to use for this. What form should that take?

@tlabaj
Copy link
Contributor

tlabaj commented May 6, 2022

@mcarrano that is a good idea. We could create some canned data that we can use in all out examples. For react the dat would maybe live alongside out dashboard wrapper. We would be able to import it into our demo code. I will open an issue for the react part. If you want some more meaningful data maybe someone could populate a spreadsheet, we could take that data and convert it into an object that our component needs..

React issue: #7384

@jenny-s51
Copy link
Contributor Author

@tlabaj @mcarrano +1, sounds like a great plan! DashboardWrapper is already very useful and is used in a lot of our table and tabs demos. Including some canned pagination data would be an awesome solution for getting the pagination to work here.

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.

LGTM! Just a small nit - any toolbar that spans the full width of a page container like that should set usePageInsets so the content in the toolbar aligns with the content in page sections and the overall page chrome. Notably what this does is add a default left/right padding of 16px, then it changes to 24px > 1200px viewport width. If you resize the page, you'll see that the header, navigation, page sections, table, data list (probably other things, too) all have this responsive padding change.

@jenny-s51
Copy link
Contributor Author

Great idea @mcoker - this logic lives in DashboardHeader and DashboardWrapper, which wraps the rest of the table (and tabs) fullscreen demos. This PR just makes use of the DashboardWrapper via an import -- changes to those files would be out of scope of the issue!

I left a comment on #7292 (comment) regarding creating a separate issue/epic to align the DashboardWrapper with core! Based on your feedback on these table PRs, there are a lot of things that need to be implemented and/or updated.

What do you think of opening up a DashboardWrapper issue alongside patternfly/patternfly-design#1157 and #7384? @tlabaj @mcoker

@tlabaj
Copy link
Contributor

tlabaj commented May 16, 2022

@jenny-s51 I opened an issue for the wrapper

@tlabaj tlabaj merged commit b992add into patternfly:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(Table): Full screen demo for Compact

7 participants