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

Edit profile #140

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

suhanichawla
Copy link
Member

@suhanichawla suhanichawla commented Apr 16, 2021

Allows user to edit their name or description. Does not allow user to edit email.
Resolves #84

Screenshot:
Screenshot (306)

@aitikgupta
Copy link
Member

(Once again, do we really need those package-lock.json changes? XD )

@aitikgupta aitikgupta added client Related to client-side code (Frontend) enhancement New feature or request server Relates to server-side code (Backend) labels Apr 17, 2021
Copy link
Member

@aitikgupta aitikgupta left a comment

Choose a reason for hiding this comment

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

Thanks for the work! 🎉

Added a few comments regarding Auth and deadly package-lock.json. XD

Comment on lines +14 to +16
const [accessToken, setAccessToken] = useState(
localStorage.getItem("access_token")
);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of getting accessToken here, you can directly use the AuthContext to get access token as well as a user (if you want) back from it. See:

const { token } = useContext(AuthContext);
const [access] = token;

And this:

<AuthContext.Provider value={{ user: [user], token: [access] }}>

Copy link
Member

Choose a reason for hiding this comment

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

Doing localStorage.getItem at multiple places might be concerning as if we ever wanted to change the key name, we'd have to do it at multiple places.

@@ -4,6 +4,7 @@
"private": true,
"dependencies": {
"@craco/craco": "^6.1.1",
"@headlessui/react": "^1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

I see we're adding it as a dependency, but the changes in package-lock.json are tooo huge to be just one package's addition? Or is that the case? :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Related to client-side code (Frontend) enhancement New feature or request server Relates to server-side code (Backend)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edit Profile in UserProfile
3 participants