Skip to content
This repository has been archived by the owner on Jan 20, 2024. It is now read-only.

Added new PR with optimized sign In and sign up functionality #64

Merged
merged 0 commits into from
Aug 15, 2022
Merged

Added new PR with optimized sign In and sign up functionality #64

merged 0 commits into from
Aug 15, 2022

Conversation

AdityaPainuli
Copy link
Contributor

Hey this one is the new PR I fixed the bugs and now you can switch between sign-in and sign-up very easily . let me know if you want any change for the google login button I am still working on it.

@vercel
Copy link

vercel bot commented Aug 14, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @ykdojo on Vercel.

@ykdojo first needs to authorize it.

@vercel
Copy link

vercel bot commented Aug 14, 2022

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

Name Status Preview Updated
defaang ✅ Ready (Inspect) Visit Preview Aug 15, 2022 at 6:35AM (UTC)

@ykdojo
Copy link
Collaborator

ykdojo commented Aug 14, 2022

@AdityaPainuli can you double-check and make sure to address all the points I raised in the previous PR?

Also, could you please use proper punctuation? I highly recommend Grammarly to check everything you write before you send it here.

@rohitdasu
Copy link
Contributor

why we need mui? we already have tailwindCSS,
not a good idea to have two things at once. @ykdojo what's your opinion?

@ykdojo
Copy link
Collaborator

ykdojo commented Aug 14, 2022

@rohitdasu right, we should probably stick with one unless they're designed to work together.

If we were to pick one, Tailwind seems like a good choice. Open to suggestions, though

@rohitdasu
Copy link
Contributor

@ykdojo tailwindCSS is enough to work with the project. If we need any components then TailwindCSS provides components as well and we can create our custom components using headless ui which is provided by TailwindCSS. So we don't need MUI or other libraries. It will make our app unnecessarily heavy.

@AdityaPainuli
Copy link
Contributor Author

Ok, I will fix the issues so do we need to create different routes for the sign-in and sign-up pages? Also, I think it will be great if the page doesn't get loaded when the user switches between those two what do you think? let me know I will do things accordingly

@AdityaPainuli
Copy link
Contributor Author

@ykdojo tailwindCSS is enough to work with the project. If we need any components then TailwindCSS provides components as well and we can create our custom components using headless ui which is provided by TailwindCSS. So we don't need MUI or other libraries. It will make our app unnecessarily heavy.

What should be the best for icons cause in hero icons which is the tailwind library I searched for google icons and couldn't able to find them. Sorry , If I am asking silly question

@iShibi
Copy link
Contributor

iShibi commented Aug 14, 2022

@AdityaPainuli Your CHANGELOG.md contains snippets of git conflicts. I suggest you to delete the file since it's already been removed from main. A better option will be to delete this fork and re-fork the repo, then create a new branch (you are working in the main of you fork currently, not recommended) and add these changed back in a new PR. This will also save you from going through fixing the git conflicts.

@rohitdasu
Copy link
Contributor

@ykdojo tailwindCSS is enough to work with the project. If we need any components then TailwindCSS provides components as well and we can create our custom components using headless ui which is provided by TailwindCSS. So we don't need MUI or other libraries. It will make our app unnecessarily heavy.

What should be the best for icons cause in hero icons which is the tailwind library I searched for google icons and couldn't able to find them. Sorry , If I am asking silly question

Don't hesitate to question, it's a good habit. Regarding the icons package if we don't get the desired icon from heroicons then we can get the icons from FontAwesome website.
NOTE: we should not install the library, instead download the icon(SVG) from FontAwesome website and put it in our public folder.
any suggestions ?
CC: @iShibi @ykdojo

@AdityaPainuli
Copy link
Contributor Author

Thanks, I will keep that in mind

@rohitdasu
Copy link
Contributor

Also this image is breaking in welcome screen

Screenshot 2022-08-14 at 10 04 35 AM

@AdityaPainuli
Copy link
Contributor Author

Ok, I will fix the issues so do we need to create different routes for the sign-in and sign-up pages? Also, I think it will be great if the page doesn't get loaded when the user switches between those two what do you think? let me know I will do things accordingly

What do you guys think about this? should I create a different page or the same page?

@AdityaPainuli
Copy link
Contributor Author

I think I put a link in it that's why this is breaking up should I download the image or is there any other way ??
image

@rohitdasu
Copy link
Contributor

I think I put a link in it that's why this is breaking up should I download the image or is there any other way ?? image

if you have permission to use that image then download and put it inside public folder

@AdityaPainuli
Copy link
Contributor Author

Ok, this Image is from unsplash.com and is available for free to use. let me correct all the issues .

@AdityaPainuli
Copy link
Contributor Author

Fixed the broken image link.

@ykdojo
Copy link
Collaborator

ykdojo commented Aug 14, 2022

For routing, first of all, a smaller PR is generally better. So can you keep the routes the same as before in this PR? In this PR, you can focus on the visual elements.

You're welcome to raise an issue, a discussion, or a PR for the routes if you think we should change them later.

Also, as I said earlier, could you please use correct punctuation in your comments for better readability?

For example, here:

Ok, this Image is from unsplash.com and is available for free to use. let me correct all the issues .

There should not be a space before the second period, and the beginning of each sentence should be capitalized.

I recommend Grammarly for this.

@AdityaPainuli
Copy link
Contributor Author

Ok sorry for the inconvenience. I will try to keep these things in my mind.

@AdityaPainuli
Copy link
Contributor Author

I created a different page for sign up and sign in as you said. Let me know what you think about it now.

@rohitdasu
Copy link
Contributor

rohitdasu commented Aug 14, 2022

i can't access the signin and signup page, I think its not deployed - @AdityaPainuli

@AdityaPainuli
Copy link
Contributor Author

AdityaPainuli commented Aug 14, 2022

Yes, it's not deployed yet. You can see the personal deployed version If you want and let me know if it is good.
https://defaang-eight.vercel.app/

Copy link
Contributor

@rohitdasu rohitdasu left a comment

Choose a reason for hiding this comment

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

please work on the changes requested

package.json Outdated
Comment on lines 16 to 17
"@mui/icons-material": "^5.8.4",
"@mui/material": "^5.10.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove MUI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will work on that.

pages/signin.tsx Outdated
@@ -1,66 +1,103 @@
import GoogleIcon from '@mui/icons-material/Google';
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use mui

@AdityaPainuli
Copy link
Contributor Author

I removed Mui completely from the project.

@rohitdasu
Copy link
Contributor

please use semantic commits - @AdityaPainuli
for more info read project's contribution guide

@AdityaPainuli
Copy link
Contributor Author

Ok, I will keep that in mind for the next time.

Copy link
Contributor

@iShibi iShibi left a comment

Choose a reason for hiding this comment

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

Import NextPage as a type so it's not included in the bundle

pages/signin.tsx Outdated Show resolved Hide resolved
pages/signup.tsx Outdated Show resolved Hide resolved
@AdityaPainuli
Copy link
Contributor Author

@ykdojo can you review the code and let me know what are your thoughts?

@AdityaPainuli
Copy link
Contributor Author

Hey, I corrected the lines you mentioned but it's saying that this branch has conflicts . Can you help me how to resolve them

@AdityaPainuli
Copy link
Contributor Author

@iShibi can you review the code and let me know what is the error?

@iShibi
Copy link
Contributor

iShibi commented Aug 15, 2022

@iShibi can you review the code and let me know what is the error?

Your package-lock.json seems to be out of sync. Delete it and run npm i. For the prettier error wait for:

@AdityaPainuli
Copy link
Contributor Author

Fixed the correction and conflicts as @ykdojo mentioned. Please review the code and let me know what you think.

Copy link
Collaborator

@ykdojo ykdojo left a comment

Choose a reason for hiding this comment

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

Added a few comments.

Another thing - in the login view, it looks squished for some reason (for example, there should be more padding between the Password label and the email box):

image

pages/signup.tsx Outdated Show resolved Hide resolved
pages/signup.tsx Outdated Show resolved Hide resolved
pages/signin.tsx Outdated Show resolved Hide resolved
pages/signin.tsx Outdated Show resolved Hide resolved
@AdityaPainuli
Copy link
Contributor Author

Applied all the corrections as you mentioned above @ykdojo

pages/signup.tsx Outdated Show resolved Hide resolved
pages/signin.tsx Outdated Show resolved Hide resolved
@AdityaPainuli
Copy link
Contributor Author

Is it okay now? Sorry I didn't understand it at the first.

next.config.js Outdated Show resolved Hide resolved
pages/signin.tsx Outdated Show resolved Hide resolved
@AdityaPainuli
Copy link
Contributor Author

As we are not using google icon right now I removed it from the project also the image link instead downloaded an image in the public folder. Is it ok now @ykdojo

@ykdojo
Copy link
Collaborator

ykdojo commented Aug 15, 2022

Should be okay now assuming the signing up and signing in still work.

I would like to wait for one or two of the other maintainers to check this first though.

pages/signup.tsx Outdated Show resolved Hide resolved
pages/signup.tsx Outdated Show resolved Hide resolved
pages/signup.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@rohitdasu rohitdasu left a comment

Choose a reason for hiding this comment

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

I've checked the auth part, it was working! but UI is what i'm not fully satisfied but it can be improved later when we have proper designs. But anyways good job @AdityaPainuli
I would suggest other maintainers also to review it. Thanks

@subhoghoshX
Copy link
Collaborator

subhoghoshX commented Aug 15, 2022

Is there any specific reason you used next router instead of the Link component?

@AdityaPainuli
Copy link
Contributor Author

No, I don't think there is any specific reason for it I just want to use it. Is there any benefit in using a link instead of a router?

@rohitdasu
Copy link
Contributor

No, I don't think there is any specific reason for it I just want to use it. Is there any benefit in using a link instead of a router?

if there is no conditional routing then we can simply use Link component.

@AdityaPainuli
Copy link
Contributor Author

Then should I change it to a link component?

@rohitdasu
Copy link
Contributor

Then should I change it to a link component?

yeah it should be changed if there is a simple route exist we should use Link only

@rohitdasu
Copy link
Contributor

also please resolve branch conflicts

@subhoghoshX
Copy link
Collaborator

I think removing it will simplify the code. Unless we're planning to redirect the user to the sign-in page after sign-up.

After signing up, if we immediately give the user access to the dashboard, then it's not necessary imo. But if we add another step in between, i.e. redirect the user to the sign-in page, then it might be useful.

@rohitdasu
Copy link
Contributor

I think removing it will simplify the code. Unless we're planning to redirect the user to the sign-in page after sign-up.

After signing up, if we immediately give the user access to the dashboard, then it's not necessary imo. But if we add another step in between, i.e. redirect the user to the sign-in page, then it might be useful.

yeah it depends to the use case

@subhoghoshX
Copy link
Collaborator

Let's merge it for now. We can refactor later. But it seems there are merge conflicts. @AdityaPainuli mind taking a look at it?

@AdityaPainuli AdityaPainuli merged commit 397bfb4 into csdojo-defaang:main Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants