Skip to content

feat(InputGroup): Add InputGroup to PF4 #1108

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
Merged
dlabaj merged 1 commit into from
Mar 4, 2019
Merged

feat(InputGroup): Add InputGroup to PF4 #1108

dlabaj merged 1 commit into from
Mar 4, 2019

Conversation

@boaz0
Copy link
Member

@boaz0 boaz0 commented Dec 27, 2018

What:

Added Patternfly 4 input group to react.

@coveralls
Copy link

coveralls commented Dec 27, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 4483

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 78.965%
Totals Coverage Status
Change from base Build 4476: 0.04%
Covered Lines: 4724
Relevant Lines: 5580
💛 - Coveralls

@patternfly-build
Copy link
Collaborator

patternfly-build commented Dec 27, 2018

PatternFly-React preview: https://1108-pr-patternfly-react-patternfly.surge.sh

dlabaj
dlabaj previously approved these changes Jan 2, 2019
View reviewed changes
@tlabaj tlabaj requested a review from christiemolloy January 4, 2019 14:50
@mcarrano
Copy link
Member

mcarrano commented Jan 7, 2019

There seem to be many variations of this component presented in Core that are not yet included here. https://pf-next.netlify.com/components/InputGroup/examples/ Is there a plan to add more variations?

@boaz0
Copy link
Member Author

boaz0 commented Jan 7, 2019

@mcarrano - sorry, I missed that part. 😱
I will add them to the examples tomorrow.

Thanks for noticing.

dlabaj
dlabaj requested changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

Styles look off when looking at what's in PF-Next. Is the component ready for consumption by react? @mcarrano

tlabaj
tlabaj reviewed Jan 11, 2019
View reviewed changes
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.

@dlabaj I had the same question on this the other day.

@boaz0
Copy link
Member Author

boaz0 commented Jan 16, 2019

@dlabaj @tlabaj - yeah I see what you're saying. I added more examples and it's clear to me the style is odd.

Could it be a problem on my side?

Thanks

christiemolloy
christiemolloy previously requested changes Jan 17, 2019
View reviewed changes
Copy link
Member

@christiemolloy christiemolloy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is inline with the CSS in pf-next , its just missing the bottom three examples. For the first three examples I can't type into the input field.

@boaz0
Copy link
Member Author

boaz0 commented Jan 17, 2019

Thanks @christiemolloy
I will fix those two problems. 👍

@boaz0
feat(InputGroup): Add InputGroup to PF4
3a461ce 
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
tlabaj
tlabaj reviewed Feb 25, 2019
View reviewed changes
packages/patternfly-4/react-core/src/components/InputGroup/examples/SimpleInputGroups.js
return (
<React.Fragment>
<InputGroup>
<Button id="textAreaButton1" variant="secondary">
Copy link
Contributor

@tlabaj tlabaj Feb 25, 2019

Choose a reason for hiding this comment

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

Can we use variant constant for these?

Copy link
Member Author

@boaz0 boaz0 Feb 26, 2019

Choose a reason for hiding this comment

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

Good idea, I will work on that.

dlabaj
dlabaj reviewed Feb 25, 2019
View reviewed changes
packages/patternfly-4/react-core/src/components/InputGroup/examples/SimpleInputGroups.js
<br />
<InputGroup>
<TextInput name="textInput10" id="textInput10" type="search" aria-label="input example with popover" />
<Popover position="top" bodyContent={'This field is an example of input group with popover'}>
Copy link
Contributor

@dlabaj dlabaj Feb 25, 2019

Choose a reason for hiding this comment

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

Same comment as @tlabaj but for position.

Copy link
Member Author

@boaz0 boaz0 Feb 26, 2019

Choose a reason for hiding this comment

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

Good point. Thanks!

@christiemolloy
Copy link
Member

christiemolloy commented Feb 25, 2019 •
edited
Loading

Only one discrepancy I can see is this example. This is what we have in Core and the button in React is a different color.

screen shot 2019-02-25 at 1 24 01 pm

I also think it would be worth making that dropdown work.

tlabaj
tlabaj approved these changes Mar 4, 2019
View reviewed changes
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 will open a issue to track remaining changes.

@boaz0
Copy link
Member Author

boaz0 commented Mar 4, 2019

@tlabaj can you give me one more day. I believe I can fix this PR according to these changes.
Please 🙏

@tlabaj tlabaj mentioned this pull request Mar 4, 2019
Closed
@tlabaj
Copy link
Contributor

tlabaj commented Mar 4, 2019

@boaz0 @christiemolloy @dlabaj I opened issue #1483 to track the remaining work.

@dlabaj dlabaj dismissed christiemolloy’s stale review March 4, 2019 15:17

Will take care of this in a seperate issue.

@dlabaj dlabaj merged commit 807ca69 into patternfly:master Mar 4, 2019
@boaz0 boaz0 deleted the inputgroup branch March 4, 2019 15:20
@boaz0
Copy link
Member Author

boaz0 commented Mar 4, 2019

No problem. I am really sorry about it. I was busy with other stuff I forgot about it.
Thank you anyway.

@priley86
Copy link
Member

priley86 commented Mar 4, 2019

this seems to have caused a regression in #1488. Will look into the type defs...

@boaz0
Copy link
Member Author

boaz0 commented Mar 4, 2019 •
edited
Loading

I know what's the problem. Didn't import ReactType from react. Writing a fix.

@mcoker mcoker mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlabaj dlabaj dlabaj approved these changes

@tlabaj tlabaj tlabaj approved these changes

+1 more reviewer

@christiemolloy christiemolloy christiemolloy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@boaz0 @coveralls @patternfly-build @mcarrano @christiemolloy @tlabaj @priley86 @dlabaj @jeff-phillips-18