-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2024-01-17] [$500] Avatar - Default Avatar photo is not fully rendered #33616
Comments
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
|
Triggered auto assignment to @robertjchen ( |
Job added to Upwork: https://www.upwork.com/jobs/~01cf187b06b1d900a9 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Hm, can't seem to reproduce this, removing |
I do not think this is a blocker, this is not blocking user from any action. @robertjchen This might require release build of the native app and its related to the expo-image PR so I am tagging @WojtekBoman @WoLewicki in case you could help handle this |
This is reproducible only on native (android/iOS). Happens on dev build as well |
@mountiny I can raise a PR asap once assigned ProposalPlease re-state the problem that we are trying to solve in this issue.Default Avatar photo is not fully rendered. What is the root cause of that problem?The formula here is wrong. The intention was to show a clear image, let's say the native width is However we currently divide by the pixel ratio, leading to the image (or svg) width and height become only 26, causing the blur. This only happens on native because only native uses the What changes do you think we should make in order to solve the problem?It should be Update: We need to add this logic to the fallback image as well, as mentioned in here What alternative solutions did you explore? (Optional)We can modify this to
so that the default avatar for workspace and user will be displayed in an |
@dukenv0307's solution makes sense. |
@dukenv0307's solution seems like would work. @situchan Sorry I didn't understand your comment. 🎀 👀 🎀 C+ reviewed. |
Current assignee @robertjchen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
@mananjadhav this is deploy blocker which means doesn't happen on production. |
@mananjadhav @situchan is right. This is stemming from another place. Let me put together a proposal so that you can review |
ProposalPlease re-state the problem that we are trying to solve in this issueAvatar - Default Avatar photo is not fully rendered What is the root cause of that problem?This is stemming from #30905, as we changed the ImageRenderer Engine from FastImage to Expo Image, and so the properties https://github.com/Expensify/App/pull/30905/files?short_path=b5e8f8f#r1436919622 Plus, we have another problem. If we use default workspace avatar, the background color will be missing in this case. This is happening due to we are using What changes do you think we should make in order to solve the problem?I am not sure that we need There are different solutions depending on the ideal behaviour
Test branch - https://github.com/shubham1206agra/App/tree/test-avatar-d What alternative solutions did you explore? (Optional) |
@shubham1206agra the Pixel ratio multiplication is so that the image has correct pixelation and looks good, if we remove it, the image will look blurry, since the device pixel ratio is high. The issue here is just that the formula we're using is incorrect. I asked @chrispader to confirm here
IMO we want this, because the attachment modal in the chat report also has zoom/pinch behavior.
@shubham1206agra I think this is a completely different issue, with a different root cause, can you report it somewhere in Slack to have another GH open? Thank you! |
ProposalPlease re-state the problem that we are trying to solve in this issue.Default avatar looks pixelated on native devices What is the root cause of that problem?We are using expo-image to render SVG images now. The logic for computing image dimension in Lightbox component considered only remote images. For sharper rendering of images on high dpi devices, it divided the native image dimension by device pixel density. App/src/components/Lightbox.js Lines 185 to 189 in d6f359f
However for SVG images this logic is not required as SVGs are vectors and could easily be resized to required width. Right now default avatars have a native width and height of 80px and dividing that by device pixel ratio further reduce the size of rendered image. That's the reason default avatars look blurred. Also right now ImageView is throwing an error for local SVG images as its prop types expect only string types. What changes do you think we should make in order to solve the problem?Instead of reducing the SVG size further by dividing its native size by device pixel ratio, it should be resized either to the <Image
source={{uri: source}}
style={imageDimensions?.lightboxSize || {width: DEFAULT_IMAGE_SIZE, height: DEFAULT_IMAGE_SIZE}}
isAuthTokenRequired={isAuthTokenRequired}
onError={onError}
onLoadEnd={() => setImageLoaded(true)}
onLoad={(e) => {
// For remote images use dpi adjusted size, for SVG resize it to fit container
const width = typeof source === 'string' ? (e?.nativeEvent?.width || 0) / PixelRatio.get() : Math.max(e?.nativeEvent?.width || 0, DEFAULT_IMAGE_SIZE);
const height = typeof source === 'string' ? (e?.nativeEvent?.height || 0) / PixelRatio.get() : Math.max(e?.nativeEvent?.height || 0, DEFAULT_IMAGE_SIZE);
setImageDimensions({...imageDimensions, lightboxSize: {width, height}});
}}
/> Also local assets uri is going to be of type number. So to fix the prop type error, we need to modify the prop types of ImageView as below. App/src/components/ImageView/propTypes.js Line 12 in d6f359f
const imageViewPropTypes = {
..
/** URL to full-sized image */
url: PropTypes.oneOfType([PropTypes.string.isRequired, PropTypes.number.isRequired]), What alternative solutions did you explore? (Optional)None (edited) |
@shubham1206agra looks like the separate issue you mentioned has its own GH created here, maybe you can move your solution there for centralized discussion. |
📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
ok, let's move forward with @dukenv0307 's proposal 👍 |
@mananjadhav The PR is ready for review. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.23-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-01-17. 🎊 For reference, here are some details about the assignees on this issue:
|
Issue is ready for payment but no BZ is assigned. @kevinksullivan you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks! |
Awaiting completion of BZ checklist |
The offending PR was already identified here and the comment was added during investigation. I don't think we need a regression test for this. This is ready for payout @kevinksullivan Can you please post the payout summary? |
Payment summary:
|
Paid @dukenv0307 , closing this one out and @mananjadhav can request in newdot. |
$500 approved for @mananjadhav based on summary above. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.17-1
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:
Expected Result:
Avatar should be fully rendered
Actual Result:
Default Avatar photo is not fully rendered.
It shows low quality image
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6326359_1703617087227.az_recorder_20231226_144911.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: