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(form): Add validated variant to form inputs #3220

Merged
merged 7 commits into from Nov 19, 2019

Conversation

@jeff-phillips-18
Copy link
Member

jeff-phillips-18 commented Oct 28, 2019

What:
Add validated variant to form inputs

Additional issues:
React follow on to: patternfly/patternfly-next#2338

closes #3091

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 28, 2019

Codecov Report

Merging #3220 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3220      +/-   ##
==========================================
+ Coverage   67.44%   67.46%   +0.02%     
==========================================
  Files         892      892              
  Lines       24872    24906      +34     
  Branches     2142     2151       +9     
==========================================
+ Hits        16774    16804      +30     
- Misses       7093     7097       +4     
  Partials     1005     1005
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 64.84% <100%> (+0.06%) ⬆️
Impacted Files Coverage Δ
...-4/react-core/src/components/TextArea/TextArea.tsx 96% <100%> (+0.54%) ⬆️
...s/patternfly-4/react-core/src/helpers/constants.ts 100% <100%> (ø) ⬆️
.../react-core/src/components/TextInput/TextInput.tsx 95.12% <100%> (+2.26%) ⬆️
...fly-4/react-core/src/components/Form/FormGroup.tsx 93.54% <100%> (+1.54%) ⬆️
...eact-core/src/components/FormSelect/FormSelect.tsx 85% <100%> (+2.64%) ⬆️
.../patternfly-react/src/components/Form/FormGroup.js 43.18% <0%> (-2.98%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 377ac47...3762929. Read the comment docs.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 28, 2019

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

1 similar comment
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 28, 2019

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

Copy link
Member

rebeccaalpert left a comment

Looks good to me.

@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Oct 28, 2019

We should add new props to demo app to verify them in integration environment.

@@ -16,6 +16,8 @@ export interface FormGroupProps extends Omit<React.HTMLProps<HTMLDivElement>, 'l
isRequired?: boolean;
/** Sets the FormGroup isValid. */
isValid?: boolean;
/** Sets the FormGroup validated. */
validated?: boolean;

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 28, 2019

Contributor

should we call this isValidated to match our other boolean modifier props?

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 28, 2019

Contributor

I was looking at this again. I think having the 2 props isValid and validated may be a little confusing. I know to make them one prop would be a breaking change. Could we maybe make validated into an enum? If the enum is used, isValid can be ignored. We can then deprecate isValid when we have a breaking change release.
ValidatedOptions { success = 'success', error = 'error', default = 'default' }
the modifiers would be applied if not default validated !== ValidatedOptions.default

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 Oct 29, 2019

Author Member

@tlabaj is this something that could be done all together for the next breaking change release? That is, leave it this way for now and change both isValid and validated to an enum value as a follow-on?

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 29, 2019

Contributor

It could. I just worry that the two props could be confusing. I wonder if we should at least make the comments more clear as to what the prop actually does. To me isValid could implies that it was validated and it might not be clear you need to also apply validated.

@@ -17,6 +17,8 @@ export interface TextAreaProps extends Omit<HTMLProps<HTMLTextAreaElement>, 'onC
isRequired?: boolean;
/** Flag to show if the TextArea is valid or invalid. */
isValid?: boolean;
/** Flag to show if the TextArea has been validated. */
validated?: boolean;

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 28, 2019

Contributor

same question as above.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Oct 28, 2019

In the <FormSelect> example, if you choose an invalid option, then choose the valid option before the validation finishes, you'll end up with this:

Screen Shot 2019-10-28 at 6 20 12 PM

And I don't see a validated example for <TextInput>, even though validated works fine.

Titani
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 5, 2019

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

1 similar comment
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 5, 2019

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

isValid?: boolean;
/** Sets the FormGroup validated. */
validated?: 'success' | 'error' | 'default';

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 Nov 5, 2019

Author Member

Maybe some explanation that success is going to add the treatments (typically not wanted).

Titani
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 5, 2019

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

@tlabaj tlabaj added the css review label Nov 6, 2019
@@ -14,13 +15,17 @@ export interface FormGroupProps extends Omit<React.HTMLProps<HTMLDivElement>, 'l
label?: React.ReactNode;
/** Sets the FormGroup required. */
isRequired?: boolean;
/** Sets the FormGroup isValid. */
/** Sets the FormGroup isValid. This prop will be deprecated you should use validated instead. */

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 7, 2019

Contributor
Suggested change
/** Sets the FormGroup isValid. This prop will be deprecated you should use validated instead. */
/** Sets the FormGroup isValid. This prop will be deprecated. You should use validated instead. */
@@ -11,8 +12,13 @@ export interface FormSelectProps
className?: string;
/** value of selected option */
value?: any;
/** Flag indicating selection is valid */
/** Flag indicating selection is valid. This prop will be deprecated you should use validated instead. */

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 7, 2019

Contributor
Suggested change
/** Flag indicating selection is valid. This prop will be deprecated you should use validated instead. */
/** Flag indicating selection is valid. This prop will be deprecated. You should use validated instead. */
@@ -15,8 +16,13 @@ export interface TextAreaProps extends Omit<HTMLProps<HTMLTextAreaElement>, 'onC
className?: string;
/** Flag to show if the TextArea is required. */
isRequired?: boolean;
/** Flag to show if the TextArea is valid or invalid. */
/** Flag to show if the TextArea is valid or invalid. This prop will be deprecated you should use validated instead. */

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 7, 2019

Contributor
Suggested change
/** Flag to show if the TextArea is valid or invalid. This prop will be deprecated you should use validated instead. */
/** Flag to show if the TextArea is valid or invalid. This prop will be deprecated. You should use validated instead. */
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.
* If set to error, text color of helper text will be modified to indicate error state.
*/
validated?: 'success' | 'error' | 'default' | ValidatedOptions;

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 7, 2019

Contributor

What is ValidatedOptions? It isn't in the docs for TextInput or Select.

This comment has been minimized.

Copy link
@tlabaj

tlabaj Nov 19, 2019

Contributor

Nice catch!

isValid?: boolean;
/** Value to indicate if the input is modified to show that validation state.
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 7, 2019

Contributor
Suggested change
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.
* If set to success, textarea will be modified to indicate valid state.
isValid?: boolean;
/** Value to indicate if the input is modified to show that validation state.
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.
* If set to error, text color of helper text will be modified to indicate error state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 7, 2019

Contributor
Suggested change
* If set to error, text color of helper text will be modified to indicate error state.
* If set to error, textarea will be modified to indicate error state.
Copy link
Contributor

mcoker left a comment

Looks good, functions as I would expect. Left some comments about the docs.

@dlabaj dlabaj requested review from christiemolloy and removed request for christiemolloy Nov 18, 2019
Titani
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 19, 2019

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

Titani
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 19, 2019

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

@@ -27,8 +27,13 @@ export interface TextInputProps extends Omit<React.HTMLProps<HTMLInputElement>,
isReadOnly?: boolean;
/** Flag to show if the input is required. */
isRequired?: boolean;
/** Flag to show if the input is valid or invalid. */
/** Flag to show if the input is valid or invalid. This prop will be deprecated you should use validated instead. */

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
/** Flag to show if the input is valid or invalid. This prop will be deprecated you should use validated instead. */
/** Flag to show if the input is valid or invalid. This prop will be deprecated. You should use validated instead. */
Titani
isValid?: boolean;
/* Value to indicate if the input is modified to shoe that validation state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
/* Value to indicate if the input is modified to shoe that validation state.
/* Value to indicate if the select is modified to show that validation state.
isValid?: boolean;
/* Value to indicate if the input is modified to shoe that validation state.
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.
* If set to success, select will be modified to indicate valid state.
isValid?: boolean;
/* Value to indicate if the input is modified to shoe that validation state.
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.
* If set to error, text color of helper text will be modified to indicate error state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
* If set to error, text color of helper text will be modified to indicate error state.
* If set to error, select will be modified to indicate error state.
isValid?: boolean;
/** Value to indicate if the input is modified to show that validation state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
/** Value to indicate if the input is modified to show that validation state.
/** Value to indicate if the textarea is modified to show that validation state.
isValid?: boolean;
/* Value to indicate if the selection is modified to shoe that validation state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
/* Value to indicate if the selection is modified to shoe that validation state.
/* Value to indicate if the input is modified to show that validation state.
isValid?: boolean;
/* Value to indicate if the selection is modified to shoe that validation state.
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.
* If set to success, input will be modified to indicate valid state.
isValid?: boolean;
/* Value to indicate if the selection is modified to shoe that validation state.
* If set to success, input will be modified to indicate valid state. If set to success, input will be modified to indicate valid state.
* If set to error, text color of helper text will be modified to indicate error state.

This comment has been minimized.

Copy link
@mcoker

mcoker Nov 19, 2019

Contributor
Suggested change
* If set to error, text color of helper text will be modified to indicate error state.
* If set to success, input will be modified to indicate valid state.
Titani
@mcoker
mcoker approved these changes Nov 19, 2019
Copy link
Contributor

mcoker left a comment

⭐️LGTM! ⭐️

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 19, 2019

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

1 similar comment
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 19, 2019

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

@dlabaj
dlabaj approved these changes Nov 19, 2019
@dlabaj dlabaj merged commit 52b17e9 into patternfly:master Nov 19, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.