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

Feature/admin login 1 #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Feature/admin login 1 #5

wants to merge 3 commits into from

Conversation

DinMon
Copy link
Owner

@DinMon DinMon commented Nov 20, 2020

Add the layout for admin to enter a password.

@VitalyKrenel
Copy link
Collaborator

VitalyKrenel commented Nov 28, 2020

image

Right now this branch is pointing to master, but it is based on the coins feature branch. Right now, when reviewing the code, I can see the things already reviewed in the feature branch.

In such cases, I suggest creating a PR pointing to the feature branch and after this feature branch gets merged, change the current PR back to master

Comment on lines +3 to +4
import BackArrowImg from '../assets/back-arrow.svg'
import ShowPasswordImg from '../assets/show-password.svg'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now these imports give you paths to the image, not components, so I would recommend using the lower case (otherwise it's a little bit confusing).

Alternatively, you can use SVGO's feature, supported by the CRA, to import SVGs as real react components:

import { ReactComponent as Logo } from './logo.svg';

function App() {
  return (
    <div>
      {/* Logo is an actual React component */}
      <Logo />
    </div>
  );
}

<form className='password-form' onSubmit={authenticateUser}>
<div className='sign-in-container'>
<div className='password-textbox-container'>
<input className='textbox password-textbox' type={(showPassword) ? 'password' : 'text'} name='password' onChange={onPasswordChange} required/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

JFYI: when you have quite a few props (or just attributes) on the component, you could split it into multiple lines to improve readability (especially for PR's viewers, but also for developers in the code):

 <input 
  className='textbox password-textbox' 
  type={(showPassword) ? 'password' : 'text'} 
  name='password' 
  onChange={onPasswordChange} 
  required
/>

)
}

export default PasswordAvatar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, when you use default exports, do you get auto completes in your VSCode?

}
}

function logUser(user) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest using signUserIn as a name, instead of logUser.

Log by itself is the same log as in console.log - on the first view of the PR I didn't spot this and thought it some debug thing.

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.

2 participants