Skip to content
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

feat(table-grid): add Table Grid component #594

Merged
merged 1 commit into from
Sep 6, 2018
Merged

feat(table-grid): add Table Grid component #594

merged 1 commit into from
Sep 6, 2018

Conversation

jeff-phillips-18
Copy link
Member

@coveralls
Copy link

coveralls commented Sep 5, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 2048

  • 29 of 29 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 78.748%
Totals Coverage Status
Change from base Build 2039: 0.1%
Covered Lines: 2911
Relevant Lines: 3446
💛 - Coveralls

serenamarie125
serenamarie125 requested changes Sep 5, 2018
View reviewed changes
Copy link
Member

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@jeff-phillips-18 this looks great, but upon hover, the column header text should also change color (consistent with PF Links)

@jeff-phillips-18 jeff-phillips-18 requested review from dtaylor113 and removed request for jschuler September 5, 2018 13:54
@jeff-phillips-18
Copy link
Member Author

@serenamarie125 Fixed

@ohadlevy
Copy link
Member

ohadlevy commented Sep 5, 2018 •
edited
Loading

should we have an hover (with full text) on long text?

@jeff-phillips-18
Copy link
Member Author

That would be up to the application to wrap the component in an overlay trigger. I thought about adding it automatically, but it would have to take all the same options plus a flag to use it or not.

dtaylor113
dtaylor113 reviewed Sep 5, 2018
View reviewed changes
...s/patternfly-3/patternfly-react-extensions/src/components/TableGrid/TableGridColumnHeader.js Outdated
return (
<Grid.Col className={classes} {...props}>
{sortable && (
<React.Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure you need the <React.Fragment> here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, correct. Leftover...

dtaylor113
dtaylor113 previously approved these changes Sep 5, 2018
View reviewed changes
serenamarie125
serenamarie125 previously approved these changes Sep 5, 2018
View reviewed changes
Copy link
Member

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM for first version thanks @jeff-phillips-18

@serenamarie125
Copy link
Member

@ohadlevy we wanted to put in minimum support so that UHC could start using this. Let me work on a doc for PF React components to support OpenShift and share with you/Liz, anyone else. Maybe your team can help with enhancing these items?

serenamarie125
serenamarie125 previously approved these changes Sep 5, 2018
View reviewed changes
Copy link
Member

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM

@jeff-phillips-18
Copy link
Member Author

@dtaylor113 Removed unnecessary React.Fragment

dtaylor113
dtaylor113 previously approved these changes Sep 5, 2018
View reviewed changes
Copy link
Member

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

  • needs rebase though

@jeff-phillips-18 jeff-phillips-18 removed the request for review from jgiardino September 6, 2018 11:39
@jeff-phillips-18
feat(table-grid): add Table Grid component
48ccc0d 
affects: patternfly-react-extensions

The table grid component is used to create a bootstrap grid based table with headers that are able to
be used for sort selection.
@dgutride dgutride merged commit c8ec177 into patternfly:master Sep 6, 2018
@dgutride dgutride removed the review label Sep 6, 2018
@serenamarie125 serenamarie125 mentioned this pull request Nov 6, 2018
Merged
@jeff-phillips-18 jeff-phillips-18 deleted the table-grid branch November 11, 2021 14:02
@tthvo tthvo mentioned this pull request Apr 5, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtaylor113 dtaylor113 dtaylor113 approved these changes

@serenamarie125 serenamarie125 serenamarie125 approved these changes

@dgutride dgutride dgutride approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jeff-phillips-18 @coveralls @ohadlevy @serenamarie125 @dgutride @dtaylor113 @priley86