-
Notifications
You must be signed in to change notification settings - Fork 93
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
Feat: Issue#27 view user personal details #38
Feat: Issue#27 view user personal details #38
Conversation
Update @anitab-org/bridgeintech-maintainers . Here's the view personal details as per issue #27 . @meenakshi-dhanani . I'm wondering what type of tests are needed for this one as I couldn't see them (tests) being mentioned in the issue's acceptance criteria. Or should it be tackle in a separate issue? |
src/myspace/MyPersonalDetails.jsx
Outdated
const [skills, setSkills] = useState(null); | ||
const [resumeUrl, setResumeUrl] = useState(null); | ||
const [photoUrl, setPhotoUrl] = useState(null); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just have const [personalDetails, setPersonalDetails] = useState(null);
and set whatever comes in response as the personalDetails?
I don't see the benefit in cluttering with so many use states
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meenakshi-dhanani , I tried your suggestion. Using const [personalDetails, setPersonalDetails] = useState(null) or useState({}) but none of these options get the data out from the response. Here's the screenshot of what I've done.
Do you have any other suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check it out, as a convention if your setter is setPersonalDetails then your variable would be personalDetails and not personal_details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @meenakshi-dhanani . I tried with the correct naming convention. Still didn't work (cannot get data out of fetch using setPersonalDetails)
- using null
- using {}
Do you have any other suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the Todo file in this sandbox - https://codesandbox.io/s/xenodochial-rain-fet6s?file=/src/Todo.js
Here, I am not setting time and title separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meenakshi-dhanani . Have you try it using the fetch from API? Does it work on your end? I've pushed the latest code with your alternative solution in case you want to give it a try. Let me know if it works for you. If you want to catch up for troubleshoot, I'm available. Ta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update @meenakshi-dhanani . Can you give other suggestion? naming convention doesn't solve the issue of not able getting data out of fetch using dict or null. So far it only works when data is set individually inside fetch as what I 've done in the PR.
src/myspace/MyPersonalDetails.jsx
Outdated
const [skills, setSkills] = useState(null); | ||
const [resumeUrl, setResumeUrl] = useState(null); | ||
const [photoUrl, setPhotoUrl] = useState(null); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @meenakshi-dhanani . I tried with the correct naming convention. Still didn't work (cannot get data out of fetch using setPersonalDetails)
- using null
- using {}
Do you have any other suggestion?
255aed8
to
a250273
Compare
src/myspace/PersonalDetails.jsx
Outdated
// const [name, setName] = useState(null); | ||
// const [ircId, setIrcId] = useState(null); | ||
// const [mentor, setMentor] = useState(null); | ||
// const [mentee, setMentee] = useState(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please even while trying no comments. If something doesn't work it's never lost. You can always get it from the previous commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. fixed now.
Update @meenakshi-dhanani . I just realized what's missing. I've now applied useEffect for fetch and it works (setting the data using dict for fetch). Sorry for the hassle. Please re-review. |
@meenakshi-dhanani. I'm going to open a new branch for testing this view user personal details. Since I'm going to carry this PR code across, I'm going to merge the 6 commits here into one to avoid |
a76b742
to
16162aa
Compare
Update @meenakshi-dhanani . I've just pushed the latest code change as we discussed just now in your office hour. Please re-review. 😉 |
src/myspace/PersonalDetails.jsx
Outdated
const requestPersonalDetails = { | ||
method: "GET", | ||
headers: { | ||
"Authorization": `Bearer ${Cookies.get("access_token")}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtreacy002 sorry I did not realize the token comes from the cookie. We are creating the AuthContext as a wrapper for any auth related details. If we start getting access_token from cookie directly, every other component that we make will have this same logic. It is the responsibility of the context, to abstract these details out.
Can you send the access_token also from the provider, and use it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. It's now added to AuthContext and used in request header from that. 👍
PR#38 rename files/routes and refactor set data using dict PR#38 apply useEffect to get data using dict PR#38 remove comments PR#38 protect personal detail page before login PR#38 Fix Navbar toggle issue after user logout PR#38 remove buttons from personal details form and fix navbar PR#38 add token to AuthContext PR#38 remove unused import PR#38 Fix navbar toggle login logout from profile pages
c0550f5
to
2e2f90a
Compare
@ramitsawhney27 I have reviewed the code, I think @foongminwong might not be available, could you test this at your end and if all looks good maybe approve? |
Description
Add feature: View user's personal details
Fixes #27
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Checklist:
Code/Quality Assurance Only