Navigation Menu

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.

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

feat(FormGroup, FormSelect, TextInput, TextArea): Add warning state #4554

Merged
merged 5 commits into from Jul 16, 2020

Conversation

tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Jul 15, 2020

What: Closes #4547

@patternfly-build
Copy link
Contributor

patternfly-build commented Jul 15, 2020

redallen
redallen previously approved these changes Jul 15, 2020
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

LGTM. Core is going to need another bump before we promote (see patternfly/patternfly#3294) and the release notes will need to be updated as well.

kmcfaul
kmcfaul previously approved these changes Jul 15, 2020
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.

@tlabaj just one thing I don't understand. On the FormSelect component, when I change the ValidatedOptions value in this line from error to warning the text below the field does not display correctly.

this.setState({ validated: ValidatedOptions.warning, invalidText: 'You must chose Three (thought that was obvious)' });

Screen Shot 2020-07-15 at 3 47 17 PM

mcoker
mcoker previously approved these changes Jul 15, 2020
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.

Looks great!!

dlabrecq
dlabrecq previously approved these changes Jul 15, 2020
@tlabaj tlabaj dismissed stale reviews from dlabrecq, mcoker, kmcfaul, and redallen via 9109162 July 15, 2020 20:31
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 line in the Validated example on Form Select is missing a "2" to get it to display the warning state when selecting the second option.

else if (value === '') { this.setState({ validated: ValidatedOptions.warning, helperText: 'You must select a value' });

Should be

else if (value === '2') { this.setState({ validated: ValidatedOptions.warning, helperText: 'You must select a value' });

mcarrano
mcarrano previously approved these changes Jul 15, 2020
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.

Please disregard my earlier comment. I misunderstood how the example is intended to work. This is approved!

mcoker
mcoker previously approved these changes Jul 15, 2020
dlabrecq
dlabrecq previously approved these changes Jul 15, 2020
@tlabaj tlabaj dismissed stale reviews from dlabrecq, mcoker, and mcarrano via 0ab45c2 July 15, 2020 21:08
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Still LGTM

@redallen redallen merged commit 4e7f69a into patternfly:master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning state to form control (text input, textarea, form select) and form helper text
7 participants