Skip to content

Commit

Permalink
Added most changes commented on PR #75.
Browse files Browse the repository at this point in the history
I've added most of the changes commented on the PR,
but I left out styles.js in SignUpPageNew since I can't
make things look nice without lots of hardcoded margins
and paddings for <Card />, as opposed to <Grid />.

Changes:
  - Got rid of !! and ?? operators in the dropdown files
  - Put schema in a separate file (schema.js)
  - Added index.js in dropdowns to export both major and
    year dropdowns with 1 import statement.
  - Put in proper changes for <Grid /> in SignUpForm so
    things look better now and I got rid of most of the
    hardcoded margins and paddings for styles.js.
  - Added clarification to what getIn() does as comments
    in the dropdown files.
  • Loading branch information
thaigillespie committed Jun 28, 2020
1 parent df153a0 commit 5afc7d3
Show file tree
Hide file tree
Showing 7 changed files with 123 additions and 78 deletions.
25 changes: 21 additions & 4 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,31 @@
"ecmaVersion": 2018,
"sourceType": "module"
},
"plugins": ["react"],
"plugins": [
"react"
],
"rules": {
"react/jsx-filename-extension": "off",
"react/jsx-props-no-spreading": "off",
"react/prop-types": [
2,
{ "ignore": ["firebase", "classes", "history", "children"] }
{
"ignore": [
"firebase",
"classes",
"history",
"children"
]
}
],
"import/no-unresolved": [

This comment has been minimized.

Copy link
@godwinpang

godwinpang Jun 28, 2020

Collaborator

why do we need to add this eslint rule?

This comment has been minimized.

Copy link
@thai-truong

thai-truong Jun 28, 2020

Collaborator

import-js/eslint-plugin-import#720
https://stackoverflow.com/questions/46208367/how-to-remove-eslint-error-no-unresolved-from-importing-react

So I was having trouble passing the linter when I changed the folder name from Dropdowns to dropdowns. Turns out there is some sort of bug or something with the linter described by the two links above. Everything matched up but it still didn't let me pass, so I added this rule to bypass it.

This comment has been minimized.

Copy link
@godwinpang

godwinpang Jun 28, 2020

Collaborator

I see - for future reference it'd be great to put these links in the commit message so that in the future people can just run git blame to see why certain rules were put in

This comment has been minimized.

Copy link
@thai-truong

thai-truong Jun 28, 2020

Collaborator

Lol okay so it passes my linter AFTER I removed the extra rule I added. Passed all the pre-commit linting and stuff as well, so irdk why it keeps failing the CircleCI build :(((

2,
{
"caseSensitive": false
}
]
},
"ignorePatterns": ["cypress"]
}
"ignorePatterns": [
"cypress"
]
}
13 changes: 10 additions & 3 deletions src/components/Dropdowns/MajorDropdown/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ const createFullMajorTitle = (department, major) => {

/*
* From https://github.com/stackworx/formik-material-ui/tree/master/packages/formik-material-ui/src (TextField) :p
* This essentially takes the props received at the top-level <Field /> as well
*
* Most of the code of this function fieldToTextField() is taken from formik-material-ui
*
* This function essentially takes the props received at the top-level <Field /> as well
* as the default props for that <Field /> (field, form, meta) and transforms
* all that into a props that is suitable for MUI's <TextField />
*/
Expand All @@ -29,15 +32,19 @@ export function fieldToTextField({
form: { isSubmitting, touched, errors },
...props
}) {
/*
* What getIn() does:
* https://stackoverflow.com/questions/56089054/how-to-dynamically-access-nested-errors-touched-on-formik-field
*/
const fieldError = getIn(errors, field.name);
const showError = getIn(touched, field.name) && !!fieldError;
const showError = getIn(touched, field.name) && fieldError != null;

return {
...props,
...field,
error: showError,
helperText: showError ? fieldError : props.helperText,
disabled: disabled ?? isSubmitting,
disabled: disabled != null ? disabled : isSubmitting,
variant: props.variant,
};
}
Expand Down
13 changes: 10 additions & 3 deletions src/components/Dropdowns/YearDropdown/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ const yearDropdownChoices = (minYear, maxYear) => {

/*
* From https://github.com/stackworx/formik-material-ui/tree/master/packages/formik-material-ui/src (TextField) :p
* This essentially takes the props received at the top-level <Field /> as well
*
* Most of the code of this function fieldToTextField() is taken from formik-material-ui
*
* This function essentially takes the props received at the top-level <Field /> as well
* as the default props for that <Field /> (field, form, meta) and transforms
* all that into a props that is suitable for MUI's <TextField />
*/
Expand All @@ -24,15 +27,19 @@ export function fieldToTextField({
form: { isSubmitting, touched, errors },
...props
}) {
/*
* What getIn() does:
* https://stackoverflow.com/questions/56089054/how-to-dynamically-access-nested-errors-touched-on-formik-field
*/
const fieldError = getIn(errors, field.name);
const showError = getIn(touched, field.name) && !!fieldError;
const showError = getIn(touched, field.name) && fieldError != null;

return {
...props,
...field,
error: showError,
helperText: showError ? fieldError : props.helperText,
disabled: disabled ?? isSubmitting,
disabled: disabled != null ? disabled : isSubmitting,
variant: props.variant,
};
}
Expand Down
4 changes: 4 additions & 0 deletions src/components/Dropdowns/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import MajorDropdown from './MajorDropdown';
import YearDropdown from './YearDropdown';

export { MajorDropdown, YearDropdown };
109 changes: 50 additions & 59 deletions src/components/SignUpForm/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,13 @@ import { withStyles } from '@material-ui/core/styles';
import { TextField } from 'formik-material-ui';

import { Formik, Field, Form } from 'formik';
import * as Yup from 'yup';

import styles from './styles';
import MajorDropdown from '../Dropdowns/MajorDropdown';
import YearDropdown from '../Dropdowns/YearDropdown';
import VALIDATION_SCHEMA from './schema';

This comment has been minimized.

Copy link
@godwinpang

godwinpang Jun 28, 2020

Collaborator

no need to be so dramatic - camel case is fine

This comment has been minimized.

Copy link
@thai-truong

thai-truong Jun 28, 2020

Collaborator

Oh okay lol. Funny cuz I thought when you said dramatic you were talking about me like irl haha. I was like dang what did I do??

Anyway, I'll change this, ty for letting me know.

This comment has been minimized.

Copy link
@godwinpang

godwinpang Jun 28, 2020

Collaborator

yeah haha not a big fan of all the ALL CAPS CONSTANTS we have around the codebase...

This comment has been minimized.

Copy link
@thai-truong

thai-truong Jun 28, 2020

Collaborator

oop I have some of them since I use all caps underscore for constants. I can change them tho if u want

This comment has been minimized.

Copy link
@godwinpang

godwinpang Jun 28, 2020

Collaborator

we dont have standards for that rn so either way haha

import { MajorDropdown, YearDropdown } from '../dropdowns';

const MIN_GRAD_YEAR = 2005;
const MAX_GRAD_YEAR = moment().year() + 4;
const PW_MIN_LENGTH = 4;

const schema = Yup.object({
email: Yup.string()
.email('Your inputted email is invalid!')
.required('Required'),
password: Yup.string()
.min(PW_MIN_LENGTH, 'Your password is too short!')
.required('Required'),
confirmPW: Yup.string().when('password', {
is: value => value && value.length > 0,
then: Yup.string()
.oneOf([Yup.ref('password')], 'Both passwords need to be the same')
.required('Required'),
}),
firstname: Yup.string().required('Required'),
lastname: Yup.string().required('Required'),
major: Yup.string()
.min(2, 'Your major is too short!')
.required('Required'),
gradYear: Yup.number().required('Required'),
});

const INITIAL_INPUT_BOX_VALUES = {
email: '',
Expand All @@ -54,19 +31,20 @@ const SignUpForm = props => {
return (
<Formik
initialValues={INITIAL_INPUT_BOX_VALUES}
validationSchema={schema}
validationSchema={VALIDATION_SCHEMA}
onSubmit={(values, { setSubmitting }) => {
handleSubmit(values, setSubmitting);
}}
>
{({ submitForm, isSubmitting }) => (
<Form>
<Grid container direction='column'>
<Grid container direction='column' spacing={3}>
<Grid
container
className={classes.nameFields}
direction='row'
spacing={3}
item
>
<Grid item xs={6}>
<Field
Expand All @@ -85,35 +63,45 @@ const SignUpForm = props => {
</Grid>
</Grid>

<Grid container direction='column'>
<Field
className={classes.vertField}
component={TextField}
name='email'
label='Email Address'
/>

<Field
className={classes.vertField}
component={TextField}
name='password'
type='password'
label='Password'
/>

<Field
className={classes.vertField}
component={TextField}
name='confirmPW'
type='password'
label='Confirm Password'
/>
<Grid container direction='column' item spacing={3}>
<Grid item>
<Field
className={classes.vertField}
component={TextField}
fullWidth
name='email'
label='Email Address'
/>
</Grid>

<Grid item>
<Field
className={classes.vertField}
component={TextField}
fullWidth
name='password'
type='password'
label='Password'
/>
</Grid>

<Grid item>
<Field
className={classes.vertField}
component={TextField}
fullWidth
name='confirmPW'
type='password'
label='Confirm Password'
/>
</Grid>
</Grid>

<Grid
container
className={classes.majorAndGradDate}
direction='row'
item

This comment has been minimized.

Copy link
@godwinpang

godwinpang Jun 28, 2020

Collaborator

personally prefer wrapping Grid container within an item to avoid ambiguity

spacing={3}
>
<Grid item xs={8}>
Expand All @@ -139,15 +127,18 @@ const SignUpForm = props => {

{isSubmitting && <LinearProgress />}

<Button
variant='contained'
color='primary'
fullWidth
disabled={isSubmitting}
onClick={submitForm}
>
Sign Up
</Button>
<Grid item>
<Button
className={classes.signUpButton}
variant='contained'
color='primary'
fullWidth
disabled={isSubmitting}
onClick={submitForm}
>
Sign Up
</Button>
</Grid>
</Grid>
</Form>
)}
Expand Down
26 changes: 26 additions & 0 deletions src/components/SignUpForm/schema.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import * as Yup from 'yup';

const PW_MIN_LENGTH = 4;

const VALIDATION_SCHEMA = Yup.object({
email: Yup.string()
.email('Your inputted email is invalid!')
.required('Required'),
password: Yup.string()
.min(PW_MIN_LENGTH, 'Your password is too short!')
.required('Required'),
confirmPW: Yup.string().when('password', {
is: value => value && value.length > 0,
then: Yup.string()
.oneOf([Yup.ref('password')], 'Both passwords need to be the same')
.required('Required'),
}),
firstname: Yup.string().required('Required'),
lastname: Yup.string().required('Required'),
major: Yup.string()
.min(2, 'Your major is too short!')
.required('Required'),
gradYear: Yup.number().required('Required'),
});

export default VALIDATION_SCHEMA;
11 changes: 2 additions & 9 deletions src/components/SignUpForm/styles.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
const styles = () => ({
nameFields: {
marginBottom: '8px',
},
vertField: {
marginBottom: '20px',
},
majorAndGradDate: {
maxWidth: '445.983px',
marginBottom: '28px',
signUpButton: {
marginTop: '10px',
},
});

Expand Down

0 comments on commit 5afc7d3

Please sign in to comment.