-
-
Notifications
You must be signed in to change notification settings - Fork 79
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
1387/form validation location #1400
1387/form validation location #1400
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
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.
Hey Brad, getting around to the review on this sunny Saturday (sidebar, lol it's foggy now as I finish it ☁️). I see you put a lot of time into this one.
Where are you at on the idea of narrowing the scope of this ticket? Ideally, when I look #1387 I'm interpreting that as primarily adding validation function and less so on CSS adaptions. I see that you put time and attention into creating another Input
component, and I'm feeling like there's room for something like ValidatedFormField
that serves the function you're calling for in Input
but still utilizes the parts of the MUI system. (there's more info in the individual comment on Input
)
I know this is commit is a work in progress still, and I'm stoked on getting validation into VRMS. This is absolutely a step in the right direction 🙌.
|
||
const handleSubmit = async (e) => { | ||
e.preventDefault(); | ||
const formSubmit = methods.handleSubmit( async (data) => { |
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.
@@ -47,6 +47,7 @@ const UserManagement = ({ users, setUserToEdit }) => { | |||
<h3>User Management</h3> | |||
<input | |||
type="text" | |||
className="text-input" |
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.
Can you please drop this change? It's outside the scope of this pull request.
@@ -24,58 +24,7 @@ import { styled } from '@mui/material/styles'; | |||
* To be used for creating and updating a project | |||
* */ | |||
|
|||
const simpleInputs = [ |
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 general I'm in favor of keeping this type of Array of Objects
style information for form generation. It forces us to make more flexible components. How do you feel about keeping the simpleInputs.map()
style and incorperating the validation requirements into the simpleInputs
object itself?
export const simpleInputs = [
{
label: 'Project Name',
name: 'name',
type: 'text',
placeholder: 'Enter project name',
validation={{
required: {
value: true,
message: 'Project name Required'
}
}}
},
...
];
Add New Project | ||
</Typography> | ||
</Box> | ||
<FormProvider {...methods}> |
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.
🙌 react-hook-form!
? 'Enter project zoom link' | ||
: 'Enter project street address' | ||
: input.placeholder | ||
<Input |
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.
See comment above, I'm in favor of Array of Objects``.map()
and would like to hear your thoughts on the perks/drawbacks between the two styles.
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.
I'd prefer we table the CSS changes. it's outside of the scope of the validation ticket.
I know the kanban board hasn't been clear on what is and isn't part of a ticket and I'm sure we'll get that worked out as we all work on this together. Where are you at on this?
Specifically, avoiding CSS Edits by using the properties of the MUI TextField
compoent and relying on the stylinging that we've put in place earlier?
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.
Can you please drop this change? It's outside the scope of this pull request.
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.
🙌
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.
Since you're not changing anything materially about this file can you please drop this edit?
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.
I apprecaite the direction this is going, however swithcing outside the MUI ecosystem loses us some important free funcationality with thigs like input highlighting, and we've done some of the heavy lifting already to adapt the theme to apply the right css to those components.
The TextArea component has support for multiline and error states :
- Error state handling via
helperText="blah"
anderror
props - MUI - TextField #validation
<TextField
error
id="outlined-error-helper-text"
label="Error"
defaultValue="Hello World"
helperText="Incorrect entry."
/>
- Multiline support via
multiline
,rows
,maxRows
props - [MUI - TextField #multiline]
<TextField
id="outlined-multiline-flexible"
label="Multiline"
multiline
maxRows={4}
/>
Can you turn this Input
component into ValidatedTextField
, pull in MUI's TextField
component, and map the error states coming from your /utils/inputValidation
to those props?
Fixes #1387
What changes did you make and why did you make them ?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied