-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add avatar preview feature #22145
Add avatar preview feature #22145
Conversation
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@venture1981 please avoid unhelpful comments on issues or PRs. |
@Puneet-here could you please resolve the conflicts |
@rushatgabhane, done! |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-09-19.at.01.49.35.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.
@Puneet-here bug: attachment preview doesn't show the image
- upload profile photo
- click view profile
Expected: attachment preview shows the uploaded photo
Actual: You see attachment icon only
Screen.Recording.2023-07-18.at.10.30.28.mov
Is there any extra step. I wasn't able to repro it Screen.Recording.2023-07-20.at.5.24.30.PM.movScreen.Recording.2023-07-20.at.5.28.02.PM.mov |
bump @rushatgabhane |
@Puneet-here no extra steps, and it's consistently reproducible. |
Let's keep this moving! What other variables could there be? different file types maybe? |
I was finally able to reproduce it. So it happens when you open it immediately. You have to be super fast. Looking into the problem. |
maybe being offline will consistently reproduce it? |
I tried a slow internet connection and also being offline but it didn't work. You just have to be very fast to repro this. I just tried checking the avatar preview in user profile page and it seems like this is reproducible there as well so I can confirm that this behaviour is already present in our attachment modal and not related to this PR. I think we should open a new ticket for this. It's really hard to repro on user profile page so here's how I would suggest reproducing it-
|
bump @rushatgabhane |
@Puneet-here could you please resolve conflicts. thx! |
Done @rushatgabhane |
@Puneet-here Looks like we had more conflicts over the weekend 😅 |
Done @rushatgabhane |
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.
Bug: mWeb safari - native attachment picker opens on clicking view photo
Screen.Recording.2023-08-09.at.22.17.47.mov
@rushatgabhane, I am not sure what the problem was but i found that it only happens when view photo option is on top. I have changed its position to last. Hope it's fine. |
@Puneet-here thanks! I think we need to dive deeper. I think |
@rushatgabhane, maybe we should get confirmation from design team. |
@rushatgabhane, I believe we can move forward with the current change since others don't think strongly about keeping the view photo as the first option? We can also keep it second in the list if we want. |
Okay, sounds good! Let's wait for merge freeze to be over |
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.
@Puneet-here the merge freeze is officially over! Let's merge with latest main
an re-test once, thanks!
Done @rushatgabhane |
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.
@stitesExpensify LGTM!
checklist - #22145 (comment)
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.
LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.72-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.72-11 🚀
|
@@ -123,6 +125,9 @@ function ProfilePage(props) { | |||
errors={lodashGet(props.currentUserPersonalDetails, 'errorFields.avatar', null)} | |||
errorRowStyles={[styles.mt6]} | |||
onErrorClose={PersonalDetails.clearAvatarErrors} | |||
previewSource={UserUtils.getFullSizeAvatar(avatarURL, accountID)} | |||
originalFileName={currentUserDetails.originalFileName} |
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.
@Puneet-here, @rushatgabhane This line is confusing, does the personal details have an originalFileName
prop? the server does not seem to return this prop and it's causing this bug #29647 .
Details
Fixed Issues
$ #11197
PROPOSAL: #11197 (comment)
Tests
Offline tests
Same
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-07-03.at.1.18.18.PM.mov
Screen.Recording.2023-07-03.at.1.17.24.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-07-05.at.2.50.36.AM.mov
Screen.Recording.2023-07-05.at.2.49.04.AM.mov
Mobile Web - Safari
Screen.Recording.2023-07-03.at.3.48.43.PM.mov
Screen.Recording.2023-07-03.at.3.45.53.PM.mov
Desktop
Screen.Recording.2023-07-03.at.3.57.40.PM.mov
Screen.Recording.2023-07-03.at.3.56.31.PM.mov
iOS
Screen.Recording.2023-07-03.at.3.55.17.PM.mov
Screen.Recording.2023-07-03.at.3.54.24.PM.mov
Android
Screen.Recording.2023-07-03.at.3.41.05.PM.mov
Screen.Recording.2023-07-03.at.3.39.50.PM.mov