-
Notifications
You must be signed in to change notification settings - Fork 44
[Hooks] Make sure the owner field is an email #396
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great :) Just minor changes:
diff --git a/src/components/HookForm/index.jsx b/src/components/HookForm/index.jsx
index d33f830..cc800b6 100644
--- a/src/components/HookForm/index.jsx
+++ b/src/components/HookForm/index.jsx
@@ -161,6 +161,7 @@ export default class HookForm extends Component {
scheduleTextField: '',
taskValidJson: true,
triggerSchemaValidJson: true,
+ validation: {},
};
static getDerivedStateFromProps(props, state) {
@@ -184,11 +185,11 @@ export default class HookForm extends Component {
taskValidJson: true,
triggerSchemaValidJson: true,
validation: {
- owner: {
+ owner: {
error: false,
- message: "",
+ message: '',
},
- }
+ },
};
}
@@ -340,13 +341,19 @@ export default class HookForm extends Component {
};
validHook = () => {
- const { hook, taskValidJson, triggerSchemaValidJson } = this.state;
+ const {
+ hook,
+ taskValidJson,
+ triggerSchemaValidJson,
+ validation,
+ } = this.state;
return (
hook.hookGroupId &&
hook.hookId &&
hook.metadata.name &&
hook.metadata.owner &&
+ !validation.owner.error &&
taskValidJson &&
triggerSchemaValidJson
);
@@ -367,16 +374,16 @@ export default class HookForm extends Component {
hook: assocPath(['metadata', 'name'], e.target.value, this.state.hook),
});
- handleOwnerChange = e =>{
+ handleOwnerChange = e => {
this.setState({
hook: assocPath(['metadata', 'owner'], e.target.value, this.state.hook),
- validation:{
+ validation: {
owner: {
error: !e.currentTarget.validity.valid,
message: e.currentTarget.validationMessage,
},
},
- })
+ });
};
handleDescriptionChange = e =>
@@ -465,7 +472,7 @@ export default class HookForm extends Component {
</ListItem>
<ListItem>
<TextField
- error = {validation.owner.error}
+ error={validation.owner.error}
required
label="Owner Email"
name="owner"
Also, if there are any linting errors, you can usually fix them via yarn lint --fix
.
@@ -168,6 +168,12 @@ export default class HookForm extends Component { | |||
scheduleTextField: '', | |||
taskValidJson: true, | |||
triggerSchemaValidJson: true, | |||
validation: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add validation: {},
on the initial state object above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I have to add the empty object, or better to instantiate it with owner prop as seen in the comment below. Im asking this because both of the comments are refering to the same code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty object is good enough.
src/components/HookForm/index.jsx
Outdated
validation: { | ||
owner: { | ||
error: false, | ||
message: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use single quotes for strings and double quotes for jsx. Use single quotes here.
src/components/HookForm/index.jsx
Outdated
@@ -344,10 +350,17 @@ export default class HookForm extends Component { | |||
hook: assocPath(['metadata', 'name'], e.target.value, this.state.hook), | |||
}); | |||
|
|||
handleOwnerChange = e => | |||
handleOwnerChange = e =>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add space before the braces.
=> {
.
}, | ||
}, | ||
}) | ||
}; | ||
|
||
handleDescriptionChange = e => | ||
this.setState({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the validHook
method, you will want to include !validation.owner.error
. See the diff.
src/components/HookForm/index.jsx
Outdated
@@ -434,9 +448,12 @@ export default class HookForm extends Component { | |||
</ListItem> | |||
<ListItem> | |||
<TextField | |||
error = {validation.owner.error} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove spaces before and after the =
symbol.
src/components/HookForm/index.jsx
Outdated
error: false, | ||
message: "", | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not meant to be removed.
validation: {}
was meant to be added above in
state = {
hook: null,
// eslint-disable-next-line react/no-unused-state
previousHook: null,
taskInput: '',
triggerSchemaInput: '',
triggerContextInput: '',
scheduleTextField: '',
taskValidJson: true,
triggerSchemaValidJson: true,
validation: {},
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out upstream was ahead of me in code. So i did not see the place you were refering to in code from my local files :) Anyway, I hope everything is Ok now :)
src/components/HookForm/index.jsx
Outdated
|
||
return ( | ||
hook.metadata.name && | ||
hook.metadata.owner && | ||
(validation.owner !== undefined ? !validation.owner.error : true) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be !validation.owner.error &&
.
src/components/HookForm/index.jsx
Outdated
error = {validation.owner.error} | ||
error={ | ||
validation.owner !== undefined ? validation.owner.error : false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error={validation.owner.error}
.
src/components/HookForm/index.jsx
Outdated
helperText={validation.owner.message} | ||
helperText={ | ||
validation.owner !== undefined ? validation.owner.message : '' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helperText={validation.owner.message}
.
getting latest version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 🥇
I modified code to use html5 input type email validation.
As the error message was not specified I added Validation message to helper text.
One thing that is strange though is that also
[a-zA-Z]+@[a-zA-Z]+
gets validated by default.I am wondering if this is expected behaviour of html5 input e-mail validation.
I hope I am on the right track :)