Skip to content

Conversation

@jenny-s51
Copy link
Contributor

@jenny-s51 jenny-s51 commented Sep 22, 2022

What: Closes #7991

Bonus updates!!:

  • Cleaned up the demo! Now 500 lines shorter/cleaner 🧼
  • Converted to function component with hooks 🪝
  • Converted to use TableComposable which is the PF recommendation over legacy table
  • Moved demo to standalone file which is the convention we are following for other PF components/demos

Demo link for convenience: https://patternfly-react-pr-8044.surge.sh/components/table/react-demos/column-management/

@patternfly-build
Copy link
Collaborator

patternfly-build commented Sep 22, 2022

@mcarrano
Copy link
Member

@jenny-s51 This looks good to me. But I'll defer to @mmenestr as I believe she worked on the original design.

@mmenestr
Copy link
Collaborator

This looks good as far as the manage column function goes! I would just add a Page header to the page because it looks a bit weird to just have breadcrumbs

Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Thanks @mmenestr , nice catch! Updated the demo to include a main section.

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.

This looks great! Wow!

It may benefit consumers to include a few more comments within the demo code to guide a consumer through the features/considerations made to implement the various bits. I found that when I read 'setFilteredRows' I began looking for the implementation of the 'Name' select filter. But that's not what 'setFilteredRows' is referring to. So, clarity, even just with additional comments, might be helpful in the fitlerData function - and maybe also to clearly separate the pagination logic from the column management logic in the code?

I'm curious if there should be a way to manage the columns in the mobile/smaller viewport screen widths? I'm not sure where the 'manage columns' button would go.

@nicolethoen
Copy link
Contributor

I also spent some time wondering if there was a better way to control column widths so that the widths dont jump around as you paginate. I'm not sure, because since the columns obviously change as they are being managed.

My only instinct is to use the width prop on columns that might change size so they are always wide enough to fit the longest content (in our case) - specifically, that means 'servers', 'status', and 'location' i think. Not sure if that would work?

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 think this is great! The added comments are super helpful and you got the column widths to work! 🤩

Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Thank you @nicolethoen for your awesome suggestions!! The additional documentation will definitely be helpful for consumers and the static column widths were a great idea 🙌

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.

Awesome work on this! 🎉 I just had a few really quick changes below. Also had the following comments:

  • When viewing the demo, two instances of "Loading..." text are being rendered:
    Double loading text in demo

    If I stop the page load in time, this is what it looks like in my dev tools:

    double loading text dev tools

    Are you by chance seeing this at all?

  • If this has already been brought up just let me know (pretty sure this topic was mentioned somewhat recently, but can't place it..), but the column widths end up truncating when there may not be a need to. If I update the table so only the URL column is visible, it still truncates despite there being plenty of space. Not necessarily a blocker, though, since it could be out of scope/something that would more be up to the consumer to handle.

@nicolethoen
Copy link
Contributor

@thatblindgeye @jenny-s51 The issue with an extra Loading... has been reported :)
patternfly/patternfly-org#3209

jenny-s51 and others added 2 commits September 29, 2022 09:37
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Thank you for your review and feedback @thatblindgeye ! Applied your code suggestions 👍

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.

Looks good! Awesome work again on this demo!

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.

LGTM

@tlabaj tlabaj merged commit 380252e into patternfly:main Sep 30, 2022
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@4.92.20
  • @patternfly/react-code-editor@4.82.20
  • @patternfly/react-console@4.92.20
  • @patternfly/react-core@4.246.0
  • @patternfly/react-docs@5.102.23
  • @patternfly/react-inline-edit-extension@4.86.20
  • demo-app-ts@4.202.1
  • @patternfly/react-log-viewer@4.87.15
  • @patternfly/react-table@4.110.20
  • @patternfly/react-topology@4.88.20
  • @patternfly/react-virtualized-extension@4.88.20

Thanks for your contribution! 🎉

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.

Table: Update column management demo data

7 participants