-
Notifications
You must be signed in to change notification settings - Fork 46
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
Jatin get profile images from website #2714
base: development
Are you sure you want to change the base?
Jatin get profile images from website #2714
Conversation
✅ Deploy Preview for highestgoodnetwork-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
PR-2714
@JatinAgrawal94
I tested the changes in the code.
I Can't see any of My photo or Suggested Profile Image Button to Display the images that match my name.
Suggestion : Also, We have to manually Enter the name otherwise it leads to the blank page. The name retrieval can be optimized to automatic retrieval from the User profile.
Hello Denish, |
Didn't understand the requirements in the code changes.
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.
Looks good, but I am not sure if the remove avatar feature is needed.
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.
Hi, I reviewed an PR #2714 @JatinAgrawal94 . I pulled the latest version to my local environment to check the fix. I'm not sure if others are experiencing the same issue, but the profile page couldn't be loaded, and errors were displayed in the browser.
Review_.2714.mov
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 reviewed the PR #2714 just now and try again, but still couldn't be loaded for the profile page.
@@ -317,6 +337,7 @@ function UserProfile(props) { | |||
try { | |||
const response = await axios.get(ENDPOINTS.USER_PROFILE(userId)); | |||
const newUserProfile = response.data; | |||
console.log(newUserProfile.profilePic) |
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 remove console.log()
<Button color="danger" onClick={toggleRemoveModal} className="remove-button"> | ||
Remove Image</Button>:<></>} | ||
{((userProfile?.profilePic==undefined || | ||
userProfile?.profilePic==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.
instead of == use ===
Code changes are looking good only having few suggestions.
Could not test it locally. |
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.
Approving the code only need below modification
- Removing console.log()
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 reviewed PR #2714 from @JatinAgrawal94 and pulled the latest version into my local environment to test the fix. However, when I tried loading the profile page, it wouldn't display, and I ran into errors in the browser. I'm not sure if anyone else is experiencing the same issue.
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.
The profile page isn't loading, and the console shows an error: TypeError: undefined is not an object (evaluating 'suggestedProfilePics.map'). I suspect this is happening in the file src/components/UserProfile/UserProfileModal/suggestedProfileModal.js, where the code attempts to use the .map() method on suggestedProfilePics, which is currently undefined.
Can you please upload the complete video for reference , starting from making required changes to the code. |
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 Jatin,
I reviewed this PR along with the related backend PR and followed the instructions you provided. I updated the server.js file by adding the following lines:
[const { getProfileImagesFromWebsite } = require('./helpers/userHelper');
getProfileImagesFromWebsite();]
After restarting the server and reloading the page (while testing by changing the name to yours), I couldn’t see any image. I’ve attached a video for your reference.
Additionally, the suggested image feature doesn’t seem to fetch relevant suggestions based on the name entered. Instead, it displays the same three images regardless of the name input.
Refer to the video below:
Screen.Recording.2024-12-11.at.11.49.41.PM.mov
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 Jatin,
I reviewed this PR along with the related backend PR and followed your instructions and recordring. After restarting the server and reloading the page (while testing by changing the from one community teams page), the image is not fetched from the website immediately but I can see the changes reflecting after some time.
As it is a cron job. Is there any schedule for the job to run due to which it is delayed?
I have attached the recording and screenshots below:
PR.2714.1.mp4
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 Jatin,
After taking your changes and following the steps mentioned, I changed the name in the user profile to match with the team member's name available on the One Community website. Then I restarted the server and waited for 5-7 minutes to refresh the profile page. On refreshing the page, I am able to see the profile image based on the user name given in the profile page.
The changes are working as expected. Please find the recording below:
Recording.2024-12-14.010607.mp4
Fixed changes, waited for re-review for too long.
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 Jatin,
I reviewed your PR again and I'm ready to approve these changes, as everything is working correctly now. Initially, I thought the changes (particularly the profile image being fetched from the One Community website) would reflect immediately, but I see now that it takes a couple of minutes.
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 @JatinAgrawal94, great work on implementing the profile image fetching feature! Here are my observations:
- Feature Highlights:
- Added ability to fetch profile images from the One Community website
- Implemented modal for suggested profile images
- Created confirmation modal for image removal
- Enhanced user profile image management
- Key Components Added:
suggestedProfileModal.js
: Modal for displaying suggested profile imagesconfirmRemoveModal.js
: Modal for confirming profile image removal- Updated
UserProfile.jsx
to support new image selection and removal workflows
- Notable Technical Implementations:
- Added endpoints for profile image management
- Implemented image conversion to base64
- Added toast notifications for image updates
- Graceful handling of profile image scenarios
- Observations & Suggestions:
- The feature seems to have a slight delay in image fetching (as noted by reviewers)
- Recommended adding error handling for image conversion
- Consider adding loading states during image fetch/update
- Testing Recommendations:
- Verify image fetching works across different user profiles
- Test edge cases like no suggested images
- Ensure smooth cross-browser compatibility
Overall, this is a solid implementation that enhances user profile customization. Great job on the detailed implementation!
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.
The profile page isn't loading for some reason. Here is the video (including the code I added following the backend PR instructions):
Description
Related PRS (if any):
This frontend PR is related to the #1116 backend PR.
Main changes explained:
confirmRemoveModal.js
tocomponents/UserProfileModal
.suggestedProfileModal.js
tocomponents/UserProfileModal
.UserProfileModal.css
andUserProfile.jsx
files.…
How to test:
npm install
andnpm run start:local
to run this PR locallysuggested Profile Image
button to display all the images that match your name.Screenshots or videos of changes:
profile_images.mp4