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

converted CreateUserForm.js to functional components #738

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

huangdarren1106
Copy link

No description provided.

@reginawang3495
Copy link
Contributor

@huangdarren1106 can you make a comment when you are ready for review? thanks!!

Copy link
Contributor

@timothycpoon timothycpoon left a comment

Choose a reason for hiding this comment

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

Good work on the refactor! Based on the test failure, I think you might have missed a setState call(s). Left a couple of other comments as well; let me know if you have any questions :)

const { username, password, confirmPassword } = this.state;
const validateInputs = () => {
// const { username, password, confirmPassword } = state;
const username1 = username;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of these 3 vars? I think you can just use username, etc

@@ -166,71 +158,64 @@ export default class CreateUserForm extends React.Component {
return addBreak ? <br /> : null;
};

updateUsername = (username) => this.setState({ username });
const updateUsername = (newusername) => setusername(newusername);
Copy link
Contributor

Choose a reason for hiding this comment

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

these 3 functions can be removed and replaced with the set functions

export default function CreateUserForm(props) {
// const init = props.initialState;
const { initialState } = props;
const [username, setusername] = useState(initialState ? initialState.username : '');
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency's sake, let's name these in camelCase, e.g. setUsername

};

const [hoverButton, sethoverButton] = useState();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in the main component body, for consistency and so that all the state variables are easily visible.

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.

4 participants