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

Conditional validation concept #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

vitkon
Copy link
Owner

@vitkon vitkon commented Mar 26, 2018

What's this PR do?

This PR solves a problem when a component needs to be conditionally validated.
For these purposes a HOC <Control> has been introduced.

Its purpose is to:

  • bind input
  • set shouldValidate flag

Example JSX:

                    <Control name="email" shouldValidate={false} {...this.props}>
                        <TextField
                            style={{ marginBottom: '20px' }}
                            label="Enter your email"
                            fullWidth={true}
                            error={!!this.dirtyInputError('email')}
                            helperText={this.dirtyInputError('email')}
                        />
                    </Control>

Where should the reviewer start?

FormContainer.tsx

How should this be manually tested?

try examples/ folder, there should be a working example there

Any background context you want to provide?

Control can be moved to a separate file, it should not reside in FormContainer file.
It was just easier for experimentation.

What are the relevant tickets / issues?

#42

Screenshots (if appropriate)

Questions

This is an experimental concept. Let me know your thoughts.

@vitkon vitkon requested a review from mattdell March 26, 2018 21:30
@vitkon vitkon changed the title adds Control HOC and shouldValidate prop Conditional validation concept Mar 26, 2018
@vitkon vitkon force-pushed the issue/42-should-validate branch 2 times, most recently from b33411b to 02f78ed Compare March 26, 2018 21:54

export interface IControlProps extends IFormProps {
name: string;
render?: (...args: any[]) => any;
Copy link
Owner Author

Choose a reason for hiding this comment

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

not used, will remove

@vitkon vitkon force-pushed the issue/42-should-validate branch from 02f78ed to eccb13f Compare March 26, 2018 22:22
@vitkon vitkon force-pushed the issue/42-should-validate branch from eccb13f to 436c9e4 Compare March 26, 2018 22:23
@@ -13,7 +14,8 @@ const makeWrapper = <T extends {}>(config: IFormConfig<T>) => (WrappedComponent:
this.state = {
model: config.initialModel || {},
touched: {},
inputs: {}
inputs: {},
shouldValidate: {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we get away without using a wrapper component and just populate shouldValidate as each bound input is rendered?

Copy link
Owner Author

Choose a reason for hiding this comment

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

what would be the best way to do it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking we could have a side effect when invoking bindInput that could update the state to register the field. Then the validation would only occur on registered fields.

That would work, but it wouldn't be able to unregister the field on unmount so that's where I draw a blank.

I like the thinking behind the approach here, but it feels like too much boilerplate. I'm trying to think if there's another way. 🤔

Copy link
Owner Author

@vitkon vitkon Mar 28, 2018

Choose a reason for hiding this comment

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

Is HOC the boilerplate you're talking about?
From my POV it's the only solution that gives us full control (e.g. we can pass ref down of not based on the child type) and allows to stay declarative.

One other thing to consider: child input might have a locked down interface (e.g. with Typescript) and won't allow for any arbitrary props like shouldValidate

What's left is to move shouldValidate to validators, but it has less access to conditional logic in JSX that can be reused.

Copy link
Owner Author

Choose a reason for hiding this comment

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

same concept in redux-form btw:
https://codesandbox.io/s/8zwvq2zr

so seems like I'm not inventing the wheel here ;)

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.

2 participants