Skip to content

Conversation

@jenny-s51
Copy link
Contributor

@jenny-s51 jenny-s51 commented Aug 17, 2022

@patternfly-build
Copy link
Collaborator

patternfly-build commented Aug 17, 2022

@tlabaj tlabaj requested review from mcarrano and mmenestr August 18, 2022 13:16
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.

This is looking good @jenny-s51 . Just a couple of small asks...

  • Is it possible to give the URL text in the last column link styling?
  • As I mentioned on another PR, if the status column could display these as labels vs text, that would be preferrable, but not a necessity.

Here's a mockup of what this ideally would look like: https://marvelapp.com/prototype/2ff7ahj6/screen/87992956

@nicolethoen nicolethoen self-requested a review August 18, 2022 14:35
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.

Since all these PRs us the same data, they would all have the same comments about its formatting as I mentioned in this comment:
#7857 (comment)
#7857 (comment)
#7857 (comment)

@jenny-s51 jenny-s51 force-pushed the stickyHeaderTableData branch from 05d2d92 to 7fc240a Compare August 22, 2022 20:04
@jenny-s51 jenny-s51 force-pushed the stickyHeaderTableData branch from 7fc240a to 509c9fe Compare August 22, 2022 21:19
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 it possible to use the dashboard wrapper in this demo?
if not, should we opt for the Masthead rather than the PageHeader?

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.

Nice catch @nicolethoen ! Updated to use the DashboardWrapper.

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

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.

Could you also clean up the unused imports?

@mcarrano
Copy link
Member

@jenny-s51 this looks much better, but just one question- What controls the column widths? It seems odd that the first 3 columns get so much space and then the columns at the end are very tight forcing 'Last modified' and 'URL' to wrap.

@jenny-s51
Copy link
Contributor Author

jenny-s51 commented Aug 23, 2022

@mcarrano I added some modifiers to the column widths as suggested by @nicolethoen here: #7857 (review)

This was to prevent the "Workspaces" and "Applications" columns from truncating like this (which is what the table would look like without the column width modifiers):
Screen Shot 2022-08-23 at 2 14 42 PM

WDYT @mcarrano ? Should we use these modifiers? Happy to update as you see fit.

@nicolethoen
Copy link
Contributor

@mcarrano I added some modifiers to the column widths as suggested by @nicolethoen here: #7857 (review)

WDYT @mcarrano ? Should we use these modifiers? Happy to update as you see fit.

Could we truncate the urls? so they take up less space and also don't wrap?

@mcarrano
Copy link
Member

Could we truncate the urls? so they take up less space and also don't wrap?

Yes, I think that makes sense.

@jenny-s51 jenny-s51 marked this pull request as draft August 23, 2022 19:23
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.

Added the truncate modifier and added fixed widths @mcarrano @nicolethoen:

Screen Shot 2022-08-23 at 3 50 36 PM

@jenny-s51 jenny-s51 marked this pull request as ready for review August 23, 2022 19:53
@nicolethoen
Copy link
Contributor

Could we make the location column just a bit wider so those values don't need to wrap?

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.

Looks great. Thanks for making these changes @jenny-s51 !

@nicolethoen nicolethoen merged commit 3191a12 into patternfly:main Aug 24, 2022
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.74.1
  • @patternfly/react-catalog-view-extension@4.86.1
  • @patternfly/react-charts@6.88.1
  • @patternfly/react-code-editor@4.76.1
  • @patternfly/react-console@4.86.1
  • @patternfly/react-core@4.235.1
  • @patternfly/react-docs@5.96.1
  • @patternfly/react-icons@4.86.1
  • @patternfly/react-inline-edit-extension@4.80.1
  • demo-app-ts@4.195.1
  • @patternfly/react-integration@4.197.1
  • @patternfly/react-log-viewer@4.80.1
  • @patternfly/react-styles@4.85.1
  • @patternfly/react-table@4.104.1
  • @patternfly/react-tokens@4.87.1
  • @patternfly/react-topology@4.82.1
  • @patternfly/react-virtualized-extension@4.82.1
  • transformer-cjs-imports@4.73.1

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.

Feat(Table): Full screen demo for Sticky Header

5 participants