Refactor Fields.tsx to use a declarative React component API#169
Refactor Fields.tsx to use a declarative React component API#169subhankar-panda wants to merge 23 commits intomasterfrom
Conversation
RedbackThomson
left a comment
RedbackThomson
left a comment
There was a problem hiding this comment.
Wow what an improvement! It is so much cleaner and easier to maintain now.
It seems that there are still a few formFields that need converting?
| }; | ||
|
|
||
| export type ApplicationInputProps = { | ||
| className?: string; |
There was a problem hiding this comment.
Is it worth creating base types (then inheriting) if they have common properties? Such as className, required, placeholder, etc.? Many fields have these properties so it'd be nice if they aren't repeated.
There was a problem hiding this comment.
Oh good point! I forgot I was going to let the base inherit from JSX.InstrisicTypes (or whatever it's called) and pass the props down to allow things like aria tags to get on the fields
There was a problem hiding this comment.
ARIA fields 😮 Are we GDPR compliant as well? 😆
There was a problem hiding this comment.
Future proofing 🤣
| type = 'text', | ||
| normalize, | ||
| placeholder, | ||
| }) => ( |
There was a problem hiding this comment.
You could pull out the fields that don't match the "Field" prop types and then just use the spread operator for the rest. eg. {errorTextInput, ...rest} then <Field component={errorTextInput} {...rest}/>.
I think that's the syntax for the spread, I'm doing this without a linter so you'll have to let me know.
| </ApplicationCol> | ||
|
|
||
| <ApplicationCol className="col-md"> | ||
| {this.createInstitutionCard(InstitutionType.UCSD, 'institution-ucsd', |
There was a problem hiding this comment.
These can probably be migrated too, right?
There was a problem hiding this comment.
I thought about it, but I think the real intent behind this refactor is to remove the nested function calls to make things more composable, didn't see a real use for "display components" to be migrated away as yet
There was a problem hiding this comment.
I agree that's the major selling point of this refactor, but having a createXYZ function that generates a component is basically just a constructor of a stateless component with extra steps and less readability.
|
|
||
| </ApplicationCol> | ||
| </ApplicationRow> | ||
| <Fields names={['institution']} component={this.showInstitutionBox} /> |
There was a problem hiding this comment.
{this.showInstitutionBox} seems hacky as well?
| 'sd-form__input-text') | ||
| )} | ||
| <ApplicationCol className="col-sm-4"> | ||
| {FormFields.createMonthPicker()} |
| ) | ||
| <ApplicationCol className="col-sm-12"> | ||
| <ApplicationLabel required={true}> | ||
| <>Why Do You Want To Attend {event.name}?</> |
There was a problem hiding this comment.
Does this require the empty blocks? I guess because it needs to be a React component to pass the type checking?
There was a problem hiding this comment.
Yes! We are passing in Why Do you want to attend {event.name}? to ApplicationLabel, and the use of {event.name} requires the surrounding text to be a React node - so I wrapped it in a Fragment, which is the intended use case anyway.
| className?: string; | ||
| forTag?: string; | ||
| // hack? | ||
| children?: string | JSX.Element[] | JSX.Element; |
There was a problem hiding this comment.
Try React.Children - https://reactjs.org/docs/react-api.html#reactchildren
Added Preview Application Feature When Application Is Closed
added button to export only emails
|
yikes what happened to this branch |
Did you pull fast-forward instead of rebase? |
|
nope, i rebased off master and fixed a bunch of conflicts, but i might have somehow rebased on an old version of master maybe? |
3 participants
I've kept some old artifacts from before the refactor eg:
createInput, createRow, createColumnbecause they are depended on by (New)EventForm that is currently being worked on in #163.I will refactor that and nuke the old code when it eventually gets merged.
Fixes #165