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

New sign up page with form completed and major and year dropdown done. #75

Merged
merged 25 commits into from
Jul 27, 2020

Conversation

thai-truong
Copy link
Collaborator

No description provided.

thaigillespie added 14 commits June 18, 2020 21:37
…ts to send in inputted data from the user to the db (need to wait for backend on that first)
…ld) and added the fix for the cypress+CircleCI bug
…ts to send in inputted data from the user to the db (need to wait for backend on that first)
…ld) and added the fix for the cypress+CircleCI bug
…EventsPage left (might have to take it out or rewrite it completely)
These two dropdown components can be put into the component prop of
Formik's <Field /> i.e. <Field component={MajorDropdown}.

Note that year dropdown needs to be supplied a minyear and a maxyear
prop for it to work properly.

Ex: <Field component={YearDropdown} minyear={#} maxyear={#} />
@thai-truong thai-truong requested a review from a team June 27, 2020 20:55
@netlify
Copy link

netlify bot commented Jun 27, 2020

Deploy preview for hkn-portal-staging ready!

Built with commit db011ea

https://deploy-preview-75--hkn-portal-staging.netlify.app

@godwinpang godwinpang linked an issue Jun 27, 2020 that may be closed by this pull request
Copy link
Collaborator

@godwinpang godwinpang left a comment

Choose a reason for hiding this comment

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

thanks for fixing theme.spacing 👍 would be nice to have it in another PR next time
everything looks good, just need to wait for integration w/ signup route to merge in

If you want to pull out the theme.spacing commits into another branch we can merge that pr in

<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<meta name="theme-color" content="#000000">
<!--
manifest.json provides metadata used when your web app is added to the
homescreen on Android. See https://developers.google.com/web/fundamentals/web-app-manifest/
-->
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this file changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm guessing indentation? Not sure why this was changed, it might have been done by VSCode

Copy link
Collaborator Author

@thai-truong thai-truong Jun 28, 2020

Choose a reason for hiding this comment

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

Okay yeah it's indentation changes made automatically by VSCode, the actual code is still the same as the original html file.

form: { isSubmitting, touched, errors },
...props
}) {
const fieldError = getIn(errors, field.name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's getIn?

Copy link
Collaborator Author

@thai-truong thai-truong Jun 28, 2020

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/56089054/how-to-dynamically-access-nested-errors-touched-on-formik-field

This is probably the only thing I found that actually explains what getIn() does lol. I don't know why there is no documentation for it on Formik's official website, even though getIn() is imported from formik.

...props
}) {
const fieldError = getIn(errors, field.name);
const showError = getIn(touched, field.name) && !!fieldError;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no bang bang hacks :) just check if it's null or sth idk what getIn returns

...field,
error: showError,
helperText: showError ? fieldError : props.helperText,
disabled: disabled ?? isSubmitting,
Copy link
Collaborator

Choose a reason for hiding this comment

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

again change to null check for future sustainability - not a big fan of ??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly that stuff is the stuff that came from formik-material-ui that I didn't touch. I'll change it tho

rsvp: PropTypes.string.isRequired,
signin: PropTypes.string.isRequired,
}),
}).isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol looking at all these long af proptypes makes me miss typescript

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

const schema = Yup.object({
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider putting schema in separate file for readability

import * as Yup from 'yup';

import styles from './styles';
import MajorDropdown from '../Dropdowns/MajorDropdown';
Copy link
Collaborator

Choose a reason for hiding this comment

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

add index.js in Dropdowns to export both so we only need 1 import statement
also change Dropdowns to lowercase to adhere with naming conventions throughout codebase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wait but I thought the convention for src/components' naming of folders is capitalization of first letter of all words in a name? That's what I see for other folders. Am I missing something here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes that's true - and perhaps I shouldn't have said naming conventions throughout codebase.

My rationale was more along the lines of React codebases having only first letter capitalized for components - since dropdowns is a folder of components it shouldnt be capitalized

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see now. Thank you so much for clarifying! Will change it accordingly

</Grid>
</Grid>

{isSubmitting && <LinearProgress />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

good

@@ -0,0 +1,14 @@
const styles = () => ({
nameFields: {
marginBottom: '8px',
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the scenarios that I can think of, we shouldn't ever ever have to hardcode any margin in terms of px

Copy link
Collaborator Author

@thai-truong thai-truong Jun 28, 2020

Choose a reason for hiding this comment

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

These bottom margins are there so that all the Formik's are spaced out evenly (and nicely) from each other. Any other way to do it? Using justify='space-evenly' does make them space out evenly, but the space between them is too small.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nvm fixed it, eliminated most hardcoded margin and padding

textTransform: 'none',
},
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

again, dont see need to hardcode margin/paddings - let's meet about this sometime

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could be missing something though - mainly just not sure about the hardcoded width (overflow small screens) and the margin/paddings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The hardcoded width is for the major dropdown box messing everything up when a major name was longer than the actual box, tho it probably shouldn't matter now because all major names are shorter than the dropdown box now (as far as I can tell).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hardcoded the logo width and height because that's the true size of the logo. Honestly all the hardcoded margins and paddings are there so the whole page looks nice, since it's a little messed up if I take them out. I'm not sure how to balance things out nicely with like how I can do with , so I guess that's why there are more hardcoded margins and paddings here.

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.
@godwinpang
Copy link
Collaborator

also looks like dropdowns import is wrong cuz of casing - build fails rn

@thai-truong
Copy link
Collaborator Author

Yeah that was the thing that I was talking about in one of your comment threads. Not sure why it happens

thaigillespie and others added 4 commits June 27, 2020 20:53
Changes:
  - <Grid component item /> now becomes:
      <Grid item>
        <Grid component>
          ...
        </Grid>
      </Grid>
  - Changed schema name from VALIDATION_SCHEMA to just 'schema'
I changed major and year dropdowns so they use Formik's <Field />
so that we don't have to use formik-material-ui's <TextField />
code to make a custom MajorDropdown or YearDropdown. This is to
prevent possible compatibility issue in the future.
const signUpSubmission = {
...values,
};

console.log(JSON.stringify(signUpSubmission));
const response = await fetch(SIGN_UP_URL, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

throw this behind something in services - frontend pages should not know anything about how we're signing up

This is to ensure that layer abstraction is maintained, so that
frontend SignUpPageNew doesn't know what we are doing for the
sign up process.
godwinpang
godwinpang previously approved these changes Jul 20, 2020
@@ -1,6 +1,10 @@
import * as firebase from 'firebase/app';
import 'firebase/auth';

import SIGNUP_ENDPOINT from '../constants/endpoints';

const SIGN_UP_URL = `http://localhost:3001${SIGNUP_ENDPOINT}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add a TODO to fetch base url based on env instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add that and then push it to newSignupPage then?

thai-truong and others added 4 commits July 23, 2020 14:50
Removed everything for the old signup page and put in the new
signup page as the current signup page. Removed new signup page
test endpoint from App/index.js and constants/routes.js.
@godwinpang
Copy link
Collaborator

Is this good? can we merge

@thai-truong
Copy link
Collaborator Author

Yeah it should be good now. I was just waiting for an approval from someone else.

@thai-truong thai-truong merged commit 1eb271e into master Jul 27, 2020
@thai-truong thai-truong linked an issue Jul 27, 2020 that may be closed by this pull request
@thai-truong thai-truong self-assigned this Aug 5, 2020
@godwinpang godwinpang deleted the newSignupPage branch August 19, 2020 19:32
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.

Integrate new SignUpPage with /signup endpoint Rewrite the sign up page completely
2 participants