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

add icons and login #1062

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

add icons and login #1062

wants to merge 4 commits into from

Conversation

YeiserGS
Copy link

to improve the user interface

Captura-desde-2022-12-12-20-06-17

@tananaev
Copy link
Member

What's the point of the mail icon?

@YeiserGS
Copy link
Author

give it a little more style

I plan to do the same in different parts of the modern web interface.

@tananaev
Copy link
Member

I personally like cleaner minimalist look. Let's do only the show password icon for now.

For any future changes please discuss first before implementing it.

@YeiserGS
Copy link
Author

Ready friend, you send it, I also have it added in the registration part so that it also shows it in the registration area. What do I do, will it be that I make a new code just by entering the password to show it?

And I also do it in the registry?

@tananaev
Copy link
Member

It doesn't look like you addressed the comment. I still see mail and other unrelated icons.

@YeiserGS
Copy link
Author

ready friend check again just apply it to what you allow me to add

@@ -57,6 +58,11 @@ const LoginPage = () => {
const [email, setEmail] = usePersistedState('loginEmail', '');
const [password, setPassword] = useState('');

const [showPassword, setShowPassword] = useState(false);
const handleClickShowPassword = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please inline these callbacks.

@@ -38,6 +39,11 @@ const RegisterPage = () => {
const [password, setPassword] = useState('');
const [snackbarOpen, setSnackbarOpen] = useState(false);

const [showPassword, setShowPassword] = useState(false);
const handleClickShowPassword = () => {
Copy link
Member

Choose a reason for hiding this comment

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

and this

Copy link
Author

Choose a reason for hiding this comment

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

ps from what it translates for me I understand that I must add the lines that you are showing me,
but I already have that added to the code

Copy link
Member

Choose a reason for hiding this comment

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

No, you should remove them and inline the code. There's no point to have separate methods with just one line implementation.

Choose a reason for hiding this comment

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

this change is missing in RegisterPage.js

type={showPassword ? 'text' : 'password'}

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.

3 participants