csdojo-defaang / defaang

A website that will curate recently-asked interview questions from FAANG+. Currently inactive. Check out: https://github.com/ykdojo/OpenStream
MIT License
509 stars 120 forks source link

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

Closed AdityaPainuli closed 2 years ago

AdityaPainuli commented 2 years ago

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[bot] commented 2 years ago

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

@ykdojo first needs to authorize it.

vercel[bot] commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

ykdojo commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

Thanks, I will keep that in mind

rohitdasu commented 2 years ago

Also this image is breaking in welcome screen

Screenshot 2022-08-14 at 10 04 35 AM
AdityaPainuli commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

AdityaPainuli commented 2 years ago

Fixed the broken image link.

ykdojo commented 2 years ago

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 commented 2 years ago

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

AdityaPainuli commented 2 years ago

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

rohitdasu commented 2 years ago

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

AdityaPainuli commented 2 years ago

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/

AdityaPainuli commented 2 years ago

I removed Mui completely from the project.

rohitdasu commented 2 years ago

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

AdityaPainuli commented 2 years ago

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

AdityaPainuli commented 2 years ago

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

ykdojo commented 2 years ago

@AdityaPainuli can you remove the Google button since it's not working yet?

For the login / signup, did you test the functionality?

ykdojo commented 2 years ago

Also, in the UI text, I spotted some grammatical mistakes. Can you check them?

AdityaPainuli commented 2 years ago

Sure I will work on that .

AdityaPainuli commented 2 years ago

I checked for the sign-in and sign-up functionality it's working fine with the database. also applied the changes you mentioned.

ykdojo commented 2 years ago

also, it looks like there are merge conflicts now

AdityaPainuli commented 2 years ago

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 commented 2 years ago

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

iShibi commented 2 years ago

@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 commented 2 years ago

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

AdityaPainuli commented 2 years ago

Applied all the corrections as you mentioned above @ykdojo

AdityaPainuli commented 2 years ago

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

AdityaPainuli commented 2 years ago

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 commented 2 years ago

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.

subhoghoshX commented 2 years ago

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

AdityaPainuli commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Then should I change it to a link component?

rohitdasu commented 2 years ago

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 commented 2 years ago

also please resolve branch conflicts

subhoghoshX commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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