-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat-#38: Implemented User Authentication UI #126
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@krishnaacharyaa , I made this PR to get some feedbacks. |
There was a problem hiding this 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
- React hook form the validation part we might have to double check all the corner cases
- Please look into indentation and alignment and font weight in mobile screen, give it a try, hopefully you'll get it...
- 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
…idth + made changes in alignment for mobile screen
There was a problem hiding this 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
Post this, we can work on the signup form, it shouldn't take much time... in my opinion, once this is sorted :) |
There was a problem hiding this 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.
frontend/src/App.tsx
Outdated
@@ -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 />} /> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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')} |
There was a problem hiding this comment.
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.
@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. |
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. |
Sounds like a plan :) |
…th pages + created auth-form-layout
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. |
After the completion of 'form element exctraction' we don't require to keep 'auth-form-layout' as a wrapper component. |
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 :) 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.
I have reverted successfully and made one more commit that for consistent routes name. |
There was a problem hiding this 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 :)
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