3
\$\begingroup\$

I've created a component that validates an email address.

It works, but since I'm new to hooks I would like some validation of what I've done before I let what I've done loose on the rest of the components I'm creating.

In particular I'm concerned I'm not managing the state correctly, it seems verbose, and I've got a lot of variables I am tracking.

If anyone has any pointers on the Fade Component as well I'd appreciate them.

I still need to handle errors back from useQuery too, I realise.

And of course, I don't know what I don't know, so feel free to point me in the right direction.

Thank you!

import React, { useState } from 'react'
import { InputAdornment, Typography } from '@material-ui/core'
import Fade from '@material-ui/core/Fade'
import CircularProgress from '@material-ui/core/CircularProgress'
import { MdCheckCircle } from 'react-icons/md'
import { ErrorMessage } from 'formik'
import TextField from 'Components/TextField'
import { string } from 'yup'
import { useAsyncFn } from 'react-use'
import { useApolloClient } from '@apollo/react-hooks'
import { FIND_BY_IDENTIFIER } from 'Queries/FindByIdentifier.query'
import { not } from 'ramda'
const schema = string()
 .ensure()
 .required('Oops, email is required')
 .email('Oops, check your email format')
const LoginInstead = () => { return (<div>Would you like to login instead?</div>) }
function FormEmailAsync ({
 name,
 email,
 handleChange,
 label,
 setFieldError,
 setFieldTouched,
 onBlurred,
 ...rest
}) {
 const [emailValid, setValidEmail] = useState('')
 const client = useApolloClient()
 const [loading, setLoading] = useState(false)
 const [error, setError] = useState(false)
 const [emailPresent, setEmailPresent] = useState(false)
 //eslint-disable-next-line no-unused-vars
 const [theState, asyncFunc] = useAsyncFn(async value => {
 const returnedSchema = await schema.validate(value)
 if (returnedSchema !== value) { return returnedSchema }
 setLoading(true)
 const { data } = await client.query({
 query: FIND_BY_IDENTIFIER,
 variables: { identifier: value },
 context: { uri: process.env.REACT_APP_GRAPHQL_PUBLIC_API }
 })
 setValidEmail(not(data.findByIdentifier.present))
 setEmailPresent(data.findByIdentifier.present) 
 setLoading(false)
 return value
 }, [])
 const onBlurFunc = async (name, email) => {
 setFieldTouched(name)
 let returnFromAsync = await asyncFunc(email) 
 if (returnFromAsync === email) {
 onBlurred(returnFromAsync)
 } else {
 setError(true)
 setFieldError(name, returnFromAsync.message)
 }
 }
 const onChangeFunc = (evt) => {
 setValidEmail(null)
 setEmailPresent(false) 
 setError(false)
 handleChange(evt) 
 }
 return (
 <div>
 <TextField
 error={error}
 name={name}
 variant='outlined'
 value={email}
 placeholder={'[email protected]'}
 onChange={evt => onChangeFunc(evt)}
 label={label}
 onBlur={event => onBlurFunc(name, event.target.value)}
 InputProps={{
 endAdornment: (
 <InputAdornment position='end'>
 <Fade
 in={loading}
 style={{
 transitionDelay: loading ? '800ms' : '0ms'
 }}
 unmountOnExit
 >
 <CircularProgress size={16} />
 </Fade>
 { emailValid === true && 
 <Fade
 in={emailValid}
 >
 <MdCheckCircle
 color='green'
 data-testid={'success'}
 size={16}
 />
 </Fade> }
 </InputAdornment>
 )
 }}
 {...rest}
 />
 { emailPresent ? LoginInstead() :
 <ErrorMessage
 name='email'
 render={msg => <Typography color='error'>{msg}</Typography> }
 />}
 </div>
 )
}
export default FormEmailAsync
```
asked Sep 7, 2019 at 23:03
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

While hooks are amazing, there is no point in using them when your function component becomes overly complex. Better to just create a class component.

Some reasons why switching to a class component is better:

  1. Defining functions within your functions will cause those variables to be redefined after each re-render which is a waste of efficiency and performance
  2. Destructuring too many variables from your useState makes it exceedingly hard to maintain in the future and hard to read

The general rule of thumb is that class components are "smart components" and function components are "dummy components". Reason why is that class components have a state and handle logic while "dummy components" are simply there to create views.

I would recommend creating a class component handling your validation, and simplifying your render method by creating "dummy components" with simple hook usage (so that they stay dumb).

answered Sep 9, 2019 at 11:22
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your review. I've spent quite some time on this hooks implementation so understandably I'm not keen to move to a class implementation. I do see some value though in creating more components to handle the "dumb" aspects. Perhaps a class might drop out of that. \$\endgroup\$ Commented Sep 11, 2019 at 1:49
1
\$\begingroup\$

I feel your component mixes a lot of concerns : API requests and responses, form field validations and error display, and presentational concerns such as styling and animations.

In my opinion, a React component works best if it concerns itself either:

  • with one concern only (API or validation or presentation)
  • with the coordination of multiple components

I think your component (and by extension, your codebase) could profit from splitting this component into multiple components or hooks that each address one of the issues. You already apply hooks very well and I don't see a problem with that – no need to switch back to class components. Hooks can even help you better with extracting logic that is not bound to a specific level in the component hierarchy, such as repeating tasks like API requests and validations.

Since you're thinking about "unleashing" this on the rest of your codebase, this is a perfect moment to think about which aspects of the component you want to reuse and how you want to use these tools, informing the design of your components.

answered Sep 9, 2019 at 13:34
\$\endgroup\$
1
  • \$\begingroup\$ Yes, I think extracting some of the components will "dry" things up and make it easier to understand what is going on. I shall try extracting the logic as you suggest into separate components and see how this looks. Thank you. :) \$\endgroup\$ Commented Sep 11, 2019 at 1:54

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.