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

Added Logout and Settings options on profile page #803

Merged
merged 18 commits into from
Jul 17, 2023
Merged

Added Logout and Settings options on profile page #803

merged 18 commits into from
Jul 17, 2023

Conversation

sumitkr2000
Copy link
Contributor

@sumitkr2000 sumitkr2000 commented Jul 14, 2023

Hey, @narayan954 I have successfully added the logout and setting options on the profile page and removed these options from the navbar. Please review my pr and merge it. Thank you!

image

@narayan954
Copy link
Owner

As I can see, you made a mistake of creating a branch from a feature branch instead of the master branch, hence the commits of that feature branch are also reflected on this branch. I can accept this pr by squashing it so that this won't reflect the commit history. Please look after this mistake from the next time.

@narayan954
Copy link
Owner

when you use git checkout to create a branch, make sure you're checking out from updated master branch. For this you may first checkout to master, update it and then create a new branch. This helps in having a clean commit history

Copy link
Owner

@narayan954 narayan954 left a comment

Choose a reason for hiding this comment

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

logout confirmation seems to be missing, can you fix that please?

@narayan954 narayan954 mentioned this pull request Jul 15, 2023
4 tasks
@sumitkr2000
Copy link
Contributor Author

As I can see, you made a mistake of creating a branch from a feature branch instead of the master branch, hence the commits of that feature branch are also reflected on this branch. I can accept this pr by squashing it so that this won't reflect the commit history. Please look after this mistake from the next time.

Okay, sorry bro for the mistake. I will not repeat this mistake from next time.

@sumitkr2000
Copy link
Contributor Author

logout confirmation seems to be missing, can you fix that please?

Yeah sure please give me some time

@sumitkr2000
Copy link
Contributor Author

@narayan954 I have added the logout modal now and it's working fine. Please review this pr.

Copy link
Owner

@narayan954 narayan954 left a comment

Choose a reason for hiding this comment

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

I'm removing the duplicate code, do let me know if there was any special reason to duplicate it

import { useSnackbar } from "notistack";

const Post = lazy(() => import("../../components/Post"));
const SideBar = lazy(() => import("../../components/SideBar"));

export function getModalStyle() {
Copy link
Owner

Choose a reason for hiding this comment

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

why were you adding this function again if you could just import it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it from app.js and then pasted it to the profile page. Later I realized that it is getting used somewhere else also. So forgot to remove it from Profile.js.

};
}

export const useStyles = makeStyles((theme) => ({
Copy link
Owner

Choose a reason for hiding this comment

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

same repeat

@narayan954 narayan954 merged commit ad8bc14 into narayan954:master Jul 17, 2023
4 of 5 checks passed
@sumitkr2000
Copy link
Contributor Author

@narayan954 Can you please add the labels?

@narayan954
Copy link
Owner

@narayan954 Can you please add the labels?

right, on it

@sumitkr2000
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants