Skip to content

Feature/admin login 1#5

Open
DinMon wants to merge 3 commits into
masterfrom
feature/admin-login-1
Open

Feature/admin login 1#5
DinMon wants to merge 3 commits into
masterfrom
feature/admin-login-1

Conversation

@DinMon
Copy link
Copy Markdown
Owner

@DinMon DinMon commented Nov 20, 2020

Add the layout for admin to enter a password.

@VitalyKrenel
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 No newline at end of file
Copy link
Copy Markdown
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
Copy Markdown
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