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

feat-#38: Implemented User Authentication UI #126

Merged
merged 25 commits into from
Apr 7, 2024

Conversation

0xmohitsen
Copy link
Contributor

@0xmohitsen 0xmohitsen commented Apr 3, 2024

Summary

This PR introduces the implementation of the signin-page UI.

Description

This PR aims to enhance the user experience by providing a dedicated signin-page UI. It uses react-hook-form + zod validation.

Images

Include any relevant images or diagrams that can help reviewers visualize the changes, if applicable

Issue(s) Addressed

Closes #38

Prerequisites

Copy link

vercel bot commented Apr 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
wanderlust ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 7, 2024 1:02pm
wanderlust-backend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 7, 2024 1:02pm

@0xmohitsen
Copy link
Contributor Author

@krishnaacharyaa , I made this PR to get some feedbacks.

@0xmohitsen 0xmohitsen changed the title feat: Implemented SignIn UI feat-#38: Implemented SignIn UI Apr 3, 2024
Copy link
Owner

@krishnaacharyaa krishnaacharyaa left a comment

Choose a reason for hiding this comment

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

It has come out really nice, just couple of things

  1. React hook form the validation part we might have to double check all the corner cases
    Screenshot_2024-04-03-23-31-34-91_40deb401b9ffe8e1df2f1cc5ba480b12
  2. Please look into indentation and alignment and font weight in mobile screen, give it a try, hopefully you'll get it...
  3. The width of complete container should be 50% width in desktop screen...
    Really appreciate the fast commits, thank you 😊
    Post this we can go ahead with the sign up form

…idth + made changes in alignment for mobile screen
@0xmohitsen
Copy link
Contributor Author

It has come out really nice, just couple of things

  1. React hook form the validation part we might have to double check all the corner cases
    Screenshot_2024-04-03-23-31-34-91_40deb401b9ffe8e1df2f1cc5ba480b12
  2. Please look into indentation and alignment and font weight in mobile screen, give it a try, hopefully you'll get it...
  3. The width of complete container should be 50% width in desktop screen...
    Really appreciate the fast commits, thank you 😊
    Post this we can go ahead with the sign up form

I made some changes.

…idth + made changes in alignment for mobile screen
Copy link
Owner

@krishnaacharyaa krishnaacharyaa left a comment

Choose a reason for hiding this comment

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

Looks almost good to me. Just couple of changes I have commented and we need changes for the mobile screen devices.
In mobile screen the heading should be 22px rest all 16px and you can play with the font-weight for bifurcation. Use the equivalent tailwind-css styles and these are the standard font sizes FYI. Feel free to explore more

frontend/src/pages/signin-page.tsx Show resolved Hide resolved
frontend/src/pages/signin-page.tsx Show resolved Hide resolved
@krishnaacharyaa
Copy link
Owner

Post this, we can work on the signup form, it shouldn't take much time... in my opinion, once this is sorted :)

Copy link
Owner

@krishnaacharyaa krishnaacharyaa left a comment

Choose a reason for hiding this comment

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

Thats great, we are almost done, just couple of observation, can you try if it is possible to achieve, else LGTM.

@@ -15,6 +17,8 @@ function App() {
<Route index element={<HomePage />} />
<Route path="add-blog" element={<AddBlog />} />
<Route path="details-page/:title/:postId" element={<DetailsPage />} />
<Route path="user-auth" element={<SignIn />} />
Copy link
Owner

Choose a reason for hiding this comment

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

Lets make it signin, so that we are consistent to signup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

)}
</div>

<div className="mb-4">
Copy link
Owner

Choose a reason for hiding this comment

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

This has a same pattern of classnames for both signin and signup pages. Can we extract a component out and reuse in both the pages?


<div className="mb-2">
<input
{...register('email')}
Copy link
Owner

Choose a reason for hiding this comment

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

Here even if we have scope to reuse the component somehow.. it would have been great refactor.

@0xmohitsen 0xmohitsen changed the title feat-#38: Implemented SignIn UI feat-#38: Implemented User Authentication UI Apr 7, 2024
@krishnaacharyaa
Copy link
Owner

@Mohitsen11 did we have any scope for the refactoring?

@0xmohitsen
Copy link
Contributor Author

0xmohitsen commented Apr 7, 2024

@Mohitsen11 did we have any scope for the refactoring?

Yeah, we can refactor.

We can see that both the files have same auth-form layout only form fields are diff.
I am working on it.

@krishnaacharyaa
Copy link
Owner

krishnaacharyaa commented Apr 7, 2024

I am working on it.

Awesome, yeah that is there , on top of it, I also am thinking if we can refactor those individual form elements like the mail, password, confirm password , username ones, almost use similar code... Can that even we somehow nicely refactor

@0xmohitsen
Copy link
Contributor Author

I am working on it.

Awesome, yeah that is there , on top of it, I also am thinking if we can refactor those individual form elements like the mail, password, confirm password , username ones, almost use similar code... Can that even we somehow nicely refactor

Yes, fields are using similar code just their types are diff.
Yeah this requires smooth refactor.
First I am trying to extract the layout logic then I'll try to nicely refactor form elements.

@krishnaacharyaa
Copy link
Owner

Sounds like a plan :)

@0xmohitsen
Copy link
Contributor Author

0xmohitsen commented Apr 7, 2024

Sounds like a plan :)

I have segregated out 'auth-form' code successfully.

'Form element extraction' work is ongoing. I am getting some issues but I'll try to make it.
Plus, from tomorrow my minors are starting.

@0xmohitsen
Copy link
Contributor Author

Sounds like a plan :)

I have segregated out auth-form code successfully.

Form element extraction work is ongoing. I am getting some issues but I'll try to make it. Plus, from tomorrow my minors are starting.

After the completion of 'form element exctraction' we don't require to keep 'auth-form-layout' as a wrapper component.

@krishnaacharyaa
Copy link
Owner

Hey @Mohitsen11, thats great efforts, thank you. I was visioning it differently actually, not as a wrapper component, but like you know just the form elements, We got this from here. I'll probably refactor and let you know as you have exams :)
Very Best for your exams. Rock it. And have to say the efforts are truely appreciated.

Can you please do me a favor, for now can you revert the previous commit. Will get this merged, and the refactoring part we can take post your exams or if I get time to do it myself, I'll just let you know :)

@0xmohitsen
Copy link
Contributor Author

0xmohitsen commented Apr 7, 2024

Hey @Mohitsen11, thats great efforts, thank you. I was visioning it differently actually, not as a wrapper component, but like you know just the form elements, We got this from here. I'll probably refactor and let you know as you have exams :) Very Best for your exams. Rock it. And have to say the efforts are truely appreciated.

Can you please do me a favor, for now can you revert the previous commit. Will get this merged, and the refactoring part we can take post your exams or if I get time to do it myself, I'll just let you know :)

Alright and thank you mentor @krishnaacharyaa .

… from auth pages + created auth-form-layout"

This reverts commit 7312064.
@0xmohitsen
Copy link
Contributor Author

Hey @Mohitsen11, thats great efforts, thank you. I was visioning it differently actually, not as a wrapper component, but like you know just the form elements, We got this from here. I'll probably refactor and let you know as you have exams :) Very Best for your exams. Rock it. And have to say the efforts are truely appreciated.

Can you please do me a favor, for now can you revert the previous commit. Will get this merged, and the refactoring part we can take post your exams or if I get time to do it myself, I'll just let you know :)

I have reverted successfully and made one more commit that for consistent routes name.

Copy link
Owner

@krishnaacharyaa krishnaacharyaa left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for all of your efforts, much appreciated. And all the best for your exams :)

@krishnaacharyaa krishnaacharyaa merged commit a4df8c2 into krishnaacharyaa:main Apr 7, 2024
3 checks passed
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.

UI - Add User Authentication Centralised Error Handling
2 participants