Geoff signup validation by GMorse19 · Pull Request #277 · sei-uxdi-collab/work_from_roam-client

GMorse19 · 2020-09-10T20:41:34Z

The functionality is there, but this PR could use some refactoring. I'll comment on some areas that I'm looking to address.

I believe any of the styling defects can be addressed when ROAM-47 is updated to reflect the new Figma drawings.

GET requests to find if an email or username is taken upon signup

Validations for signUp fields and corresponding messages

Need to work on styling

Needed to drop the dropdown button as it conflicted with PasswordInput

GMorse19 · 2020-09-10T20:42:43Z

  "scripts": {
    "start": "react-scripts start",
-    "build": "react-scripts build",
+    "build": "CI= react-scripts build",


Is this needed? I changed this while testing out netlify.

what does this line do?

When I was originally trying to get netlify working, I found this fix. Apparently it prevents warnings from creating a fatal error in the build process. This link explains it some.

GMorse19 · 2020-09-10T20:44:57Z

+      property: document.documentElement.style.setProperty('--border-show', 'none')
    }
  }
+  // Set state of all fields using helper functions


I was looking into a cleaner way to do this. The checkValid() function checks the state of all the fields at once. I'm looking into just checking the specific fields that are being altered. Maybe a helper function? Or possibly a naming convention that would allow for easier refactoring?

GMorse19 · 2020-09-10T20:51:56Z

      .then(() => history.push('/first-signin'))
      .catch(error => {
        console.error(error)
-        this.setState({ email: '', username: '', password: '', passwordConfirmation: '' })


This cleared the form after an error. I removed it so the user has an opportunity to alter their inputs.

GMorse19 · 2020-09-10T20:54:09Z

            <TextField
              fullWidth={true}
-              className="account-info"
+              className={!usernameVal ? 'account-info-signup-red username input' : 'account-info-signup username input'}


I'm using the class here to change the color of the form border. Red for invalid and Green for Valid. This will most likely be unnecessary when we implement the new figma drawings.

GMorse19 · 2020-09-10T20:58:24Z

+                src={'red-x.svg'}
+                alt='red-x'
+                className={!usernameVal ? 'red-x' : 'green-check'}
+                onMouseEnter={!usernameVal ? () => this.onHover(this.state.openUser, 'openUser') : undefined}


I'm using hover to display the error messages. It works well for the web browser, but we may want to implement an onClick also. For mobile it may be essential.

asooge · 2020-09-11T01:08:22Z

  "scripts": {
    "start": "react-scripts start",
-    "build": "react-scripts build",
+    "build": "CI= react-scripts build",


what does this line do?

asooge · 2020-09-11T01:12:24Z

+      passwordNumber: false,
+      passwordConfirmation: '',
+      passwordConfirmationVal: false,
+      property: document.documentElement.style.setProperty('--border-show', 'none')


what is document.documentElement.style.setProperty('--border-show', 'none')?

I meant to comment on this. I'm using a CSS variable to change the border from 'none' to 'show' upon submit. This line resets the border display to 'none' when first loading the signup page. Without it, if you left the signup page and came back the borders would still be showing. We probably won't need the CSS variable when we implement the newer wire frames, but I wanted to leave it in for now... just in case.

If you want this code to execute on load, why don't we add this to componentDidMount()

Do we need to save property to state?

I'm setting state on line 83
this.setState({ property: document.documentElement.style.setProperty('--border-show', 'solid') })
Do you think it would still be a better idea to use componentDidMount()?

I was planning on addressing this issue when tackling the new wireframes. I'm not 100% sure we'll need this functionality at all. Most likely, we can just remove it.

asooge · 2020-09-11T01:20:41Z

-  handleChange = event => this.setState({
-    [event.target.name]: event.target.value
-  })
+  handleChange = event => {


This code is difficult to follow and somewhat repetitive. How can we refactor? What are we trying to accomplish with this function?

I wanted the ability to check username and email while the user is typing. I only need to do this on two of the four form fields. The other two form fields still need the checkValid function. I went with the conditional to get this to work, but I would love to clean it up some.

The two api calls for checkname() and checkemail() are what drove me to do it this way originally. I may be able to rename the functions and condense down one of the conditionals, but I'll have to play with some ideas. I'm open to suggestions.

I refactored the api calls so we can send two arguments. This allowed for the refactoring of handleChange a bit.

Maybe we can still do something with () => { this.checkValid()? I was toying with the idea of placing it once at the end of the function, but I'm having trouble thinking of a way to call the api only when necessary.

asooge · 2020-09-11T01:36:58Z

+}
+export const passwordConfirmationTest = (pw, pw2) => {
+  return pw === pw2
+}


can we consolidate this into a single function that will check all inputs and return an object with the relevant errors? For example, if invalid username length but everything else okay, return { usernameLength: true }

Then call this function on every input change (handleChange) and update this.state.errors with the output

Yes. I will try something along those lines.

I began dabbling with this issue a little and came across this link.

It discusses the pros and cons of an object of functions or multiple functions. It doesn't solve this particular issue, but I thought it was interesting. It did lead to this bit of brilliance, though.

This

import { emailTest, emailValid, usernameTest, usernameLength, passwordTest, passwordLength, passwordCapital, passwordLower, passwordSpecial, passwordNumber, passwordConfirmationTest } from '../../helpers/signUpValidation'

To This

import * as validations from '../../../helpers/signUpValidation'

openPass, openEmail and openUser were unnecessary

GMorse19 added 12 commits August 27, 2020 22:52

Added checkemail and checkusername functions

e747fa6

GET requests to find if an email or username is taken upon signup

Sign up validation messsages

a642dd5

Added signUpValidation.js

a797bdb

Validations for signUp fields and corresponding messages

Initial setup for signup validations

0227f53

Validations working

d2aa40e

Need to work on styling

Working on styling issue

6e214ad

Working on styling issue

f5e8800

Drop down malfunctioning

60fe523

Signup validations working

3356e56

Needed to drop the dropdown button as it conflicted with PasswordInput

Altered package.json for Netlify

62b913f

Signup messaging operational

5001a38

Refactored hover function

082129c

GMorse19 added the help wanted Extra attention is needed label Sep 10, 2020

GMorse19 self-assigned this Sep 10, 2020

GMorse19 commented Sep 10, 2020

View reviewed changes

asooge reviewed Sep 11, 2020

View reviewed changes

GMorse19 added 6 commits September 11, 2020 00:39

Refactored validations import

c4c8678

Refactored checkemail and checkusername into one function

adf2f69

Refactored handleChange

4d08188

CSS refactor and clean up

76316f3

Refsactored state

05d7611

openPass, openEmail and openUser were unnecessary

Comment

0f902fc

Uh oh!

Conversation

GMorse19 commented Sep 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants