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

fix-#432: Bug Fixed #438

Conversation

kapilG0
Copy link
Contributor

@kapilG0 kapilG0 commented Jun 27, 2024

Summary

Fixed header for signup and login page in mobile

Description

Header fixed for signup and login page

Images

image
image

Issue(s) Addressed

Closes #432

Prerequisites

Copy link

vercel bot commented Jun 27, 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 Jun 28, 2024 0:25am
wanderlust-backend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 28, 2024 0:25am

Copy link

Hey @kapilG0! Thanks for sticking to the guidelines! High five! 🙌🏻

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.

Thank you @kapilG0 , Kindly address the review comments and add snapshots of the desktop view even to cross check.

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

@kapilG0 take some time, and go through https://tailwindcss.com/docs/responsive-design , and get back,
The solution shouldn't be more than 6 to 8 characters meaning it is that minor change, you don't have to create new custom classes and stuff. And forget about the tablet screen,

The task is not to the existing code base of desktop, only in the mobile it needs to be fixed, figure it out

@kapilG0
Copy link
Contributor Author

kapilG0 commented Jun 28, 2024

@kapilG0 take some time, and go through https://tailwindcss.com/docs/responsive-design , and get back, The solution shouldn't be more than 6 to 8 characters meaning it is that minor change, you don't have to create new custom classes and stuff. And forget about the tablet screen,

The task is not to the existing code base of desktop, only in the mobile it needs to be fixed, figure it out

Hello @krishnaacharyaa based on above documentation i changed code .please check below images if it is fine then i will raise pr.
for mobile
image
for ipad
image
for desktop
image

@krishnaacharyaa
Copy link
Owner

krishnaacharyaa commented Jun 28, 2024

Hello @krishnaacharyaa based on above documentation i changed code

Kindly push it, so that I can tell if you are on the right track

@kapilG0
Copy link
Contributor Author

kapilG0 commented Jun 28, 2024

Hello @krishnaacharyaa based on above documentation i changed code

Kindly push it, so that I can tell if you are on the right track

i have pushed please check

@@ -68,7 +67,7 @@ function Signup() {
<div className="flex-grow cursor-default bg-white py-4 dark:bg-dark-card">
<div className="m-4 mb-4 flex justify-center">
<div className="flex w-full items-center justify-center">
<h2 className="w-3/4 pl-48 text-center text-lg font-bold text-black dark:text-dark-primary sm:text-xl">
<h2 className="text-center text-lg font-bold text-black dark:text-dark-primary sm:w-2/4 sm:pl-2 sm:text-xl lg:w-3/4 lg:pl-48">
Sign up to WanderLust
Copy link
Owner

Choose a reason for hiding this comment

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

you mean to say w-2/4 text-xl md:w-3/4 md:pl-48?

You are almost there, figure out why i said the above instead of sm:w-2/4 sm:pl-2 sm:text-xl lg:w-3/4 lg:pl-48

ps: We are focusing on mobile and devices with greater than mobile width screen . i.e md: and default should only be used, you can see the repo, no where we are using the lg:, you have solution here, please change it and try to understand why this change i suggested :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean to say w-2/4 text-xl md:w-3/4 md:pl-48?

You are almost there, figure out why i said the above instead of sm:w-2/4 sm:pl-2 sm:text-xl lg:w-3/4 lg:pl-48

ps: We are focusing on mobile and devices with greater than mobile width screen . i.e md: and default should only be used, you can see the repo, no where we are using the lg:, you have solution here, please change it and try to understand why this change i suggested :)

ok now i understood by default it will be desktop and for mobile and screen small than desktop we should add sm,md for that right ?

Copy link
Owner

Choose a reason for hiding this comment

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

ok now i understood by default it will be desktop and for mobile and screen small than desktop we should add sm,md for that right ?

Default is for mobile @kapilG0 in tailwind-css, in bootstrap it was what you are saying

Copy link
Contributor Author

@kapilG0 kapilG0 Jun 28, 2024

Choose a reason for hiding this comment

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

ok now i understood by default it will be desktop and for mobile and screen small than desktop we should add sm,md for that right ?

Default is for mobile @kapilG0 in tailwind-css, in bootstrap it was what you are saying

oh ok. thank you for clearing and helping me a lot in that . i have never use tailwind i will check some resource for that. i am modifying my changes . very sorry

Copy link
Owner

Choose a reason for hiding this comment

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

No problem @kapilG0 , the hunger for learning, motivated me to help you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem @kapilG0 , the hunger for learning, motivated me to help you :)

thanks @krishnaacharyaa

@@ -73,7 +72,7 @@ function signin() {
<div className="flex-grow cursor-default bg-white py-4 dark:bg-dark-card">
<div className="m-4 mb-4 flex justify-center">
<div className="flex w-full items-center justify-center">
<h2 className="w-3/4 pl-48 text-center text-lg font-bold text-black dark:text-dark-primary sm:text-xl">
<h2 className="text-center text-lg font-bold text-black dark:text-dark-primary sm:w-2/4 sm:pl-2 sm:text-xl md:w-3/4 md:pl-48">
Sign in to WanderLust
Copy link
Owner

Choose a reason for hiding this comment

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

As discussed you would have to remove sm: because mobile screen would be less with than sm: and we need this for mobile screen right? which is default. so why do we need to add sm: correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed you would have to remove sm: because mobile screen would be less with than sm: and we need this for mobile screen right? which is default. so why do we need to add sm: correct?

yup understood @krishnaacharyaa

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.

Thank you LGTM

@krishnaacharyaa krishnaacharyaa merged commit db4b416 into krishnaacharyaa:main Jun 28, 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.

[BUG] header should be displayed properly in signup and login page
2 participants