Skip to content

Conversation

@jenny-s51
Copy link
Contributor

@jenny-s51 jenny-s51 commented May 5, 2022

@patternfly-build
Copy link
Collaborator

patternfly-build commented May 5, 2022

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.

Is the sortable columns in the core example something we should add to the react example? I notice the compound expandable example (not the demo) does not have sortable columns, so I'm not sure how hard that'd be to wire up...
What do you think, @mcarrano ?

@mcarrano
Copy link
Member

mcarrano commented May 6, 2022

It seems like sortable columns and compound expansion and really demonstrating two separate things. So I'm OK to leave it out of this demo unless there is value in including it here because there are unique issues to integrating column sorting with compound expansion that do not occur elsewhere.

@mmenestr
Copy link
Collaborator

mmenestr commented May 6, 2022

Something random I noticed is that clicking on the "Open in GitHub" link on the first row, opens up the second column expansion. That probably shouldn't happen right?

@jenny-s51
Copy link
Contributor Author

jenny-s51 commented May 6, 2022

It seems like sortable columns and compound expansion and really demonstrating two separate things. So I'm OK to leave it out of this demo unless there is value in including it here because there are unique issues to integrating column sorting with compound expansion that do not occur elsewhere.

@mcarrano I agree that it makes the most sense to leave sorting out of this demo, since we are demonstrating compound expansion and don't want to puff up the demo too much with repeated code that doesn't directly relate to the table variant being demoed.

If consumers want to see an example of sorting, they can check out our sortable demo!.

Something random I noticed is that clicking on the "Open in GitHub" link on the first row, opens up the second column expansion. That probably shouldn't happen right?

Good question and nice catch @mmenestr ! For all our react demos with an action link, the href is set to #. When the link is clicked, the # tag is added to the url and the component is rerendered.

This effectively has the same function as refreshing the page, which is why the row gets expanded again (that's the default state here). This won't present any issues to consumers.

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.

I would make this a little more similar to core by

  1. Chang the name to compound expansion
  2. add a toolbar
  3. add bottom pagination

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

@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!

@tlabaj tlabaj merged commit 2283b01 into patternfly:main May 18, 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 compound expansion

7 participants