App
App copied to clipboard
Expensify
Log in - Avatar with user's email are very close to each other
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
1.Go to https://staging.new.expensify.com/ 2. In the email field enter a email that has an active Expensify account and click on continue 3. Disable the internet connection in the device 4. Verify the Sign in button is disabled and there's a message indicating the user is offline. 5. Verify the link "Not ? Go back." is still enabled when offline 6. Click/tap on “Forgot?”
Expected Result:
Avatar with user's email have some extra space between each other
Actual Result:
Avatar with user's email are very close to each other
Workaround:
Unknown
Platform:
Where is this issue occurring?
- iOS
- Mobile Web
Version Number: 1.2.26.0
Reproducible in staging?: Yes
Reproducible in production?: yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/93399543/201146995-6a65bcbb-3b58-4ba3-ba60-e00af886ae92.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
@michaelhaxhiu lucky you - I'm about to go ooo and don't want this blocked on me
Proposal
https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/components/Avatar.js#L64-L82
When offline, image fetch fails and this.state.imageError set to true
But Icon doesn't have any style set, while Image has imageStyle
Solution:
We should apply margin to both Icon and Image
https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/pages/signin/ResendValidationForm.js#L58-L60
- imageStyle={[styles.mr2]}
+ containerStyles={[styles.mr2]}
@michaelhaxhiu Whoops! This issue is 2 days overdue. Let's get this updated quick!
Current assignee @michaelhaxhiu is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)
Triggered auto assignment to @sketchydroide (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
Job posted to upwork btw - https://www.upwork.com/jobs/~01ef6eb70da98d904e
Cant recreate the issue in any env. and the stack overflow post seems inaccessible to me as i dont have a @expensify email
Attemtped local on Iphone 13 from both native and browser
So I tested this a bit, and this only works if the user presses the button while in airplane mode. Once I test this while connected, it no longer happens even if I go back to airplane mode.
@markarmanus have you followed the steps exactly? Even with the airplane mode? I was just able to replicate myself. See my comment above
Proposal
@sketchydroide I can recreate the issue scuccefully.
The issue is really simple, When this.state.imageError is true in Avatar.js
{_.isFunction(this.props.source) || this.state.imageError
? (
<Icon
src={this.state.imageError ? this.props.fallbackIcon : this.props.source}
height={iconSize}
width={iconSize}
fill={this.state.imageError ? themeColors.offline : this.props.fill}
/>
)
: (
<Image
source={{uri: this.props.source}}
defaultSource={getAvatarDefaultSource(this.props.source)}
style={imageStyle}
onError={() => this.setState({imageError: true})}
/>
)}
we notice that Image is getting the style ImageStyle while Icon is not.
ImageStyle is defined as
const imageStyle = [
StyleUtils.getAvatarStyle(this.props.size),
...this.props.imageStyles,
];
which is applying the this.props.imageStyles. This is what includes the margin.
To solve the issue we need to apply the style to Icon. considering Icon does not take a container style prop. We can wrap it in a View and pass it the imageStyle
<View style={this.props.imageStyles}>
<Icon
src={this.state.imageError ? this.props.fallbackIcon : this.props.source}
height={iconSize}
width={iconSize}
fill={this.state.imageError ? themeColors.offline : this.props.fill}
/>
</View>
Before
After
@markarmanus Sslution looks good to me. But there is an issue with it. StyleUtils.getAvatarStyle(this.props.size) also applies a background color, if you notice carefully after putting this solution the icon becomes darker so we will have to scrap the background color when the icon is being used.
Note: @markarmanus taking this issue means that you will have to test the whole app where the Avatar component is used and add a video showing that. It should not break anything.
we can do that in PR.
cc: @sketchydroide
:ribbon: :eyes: :ribbon: C+ reviewed
@markarmanus are you ok with what @parasharrajat said about testing every where the Avatar component is used? As soon as you let me know I'll assign this to you :)
@sketchydroide Yes sir :D that sounds perfect with me !
I sent an email to join the slack channels but never got a response back
Yes, I found https://github.com/Expensify/App/issues/12645#issuecomment-1318981321 better long-term solutions. There might be other pages with a similar issue. All will be knocked off at once.
If Avatar component needs margin in any parent component, containerStyles can be directly used.
imageStyle can be used for image's own style definition.
And my solution doesn't need regression test on any other pages
At first, I didn't see it. But during recent testing, I reviewed that too. It is working no doubt.
Yes, I found #12645 (comment) better long-term solutions. There might be other pages with a similar issue. All will be knocked off at once.
I checked Avatar usages throughout the app.
Except this page (ResendValidationForm), all margin styles are defined in containerStyles
And imageStyle is only used for width, height set in other Avatar usages
I personally think that imageStyle should be applied to the icon as well because the icon is also used as an image. It does not need to have separate styles. If the user is using imageStyles it should be the same for icon part as well.
Rest, I leave the decision to @sketchydroide. FYI, https://github.com/Expensify/App/issues/12645#issuecomment-1312704144 proposal is also working fine.
@parasharrajat @0xmiroslav Your solution would 100% work. but if we follow SOLID princibles. The Avatar component is responsible for honouring its interface. In this case the interface says imageStyles is what should apply to the image. (regardless of intenral details) and if its not being applied like in this situation when this.state.imageError is true then the component is not designed properly.
https://github.com/Expensify/App/blob/7de9ab2d62849fc670879a680dd46cf8003697f8/src/components/Avatar.js#L17-L22
When someone wants to use Avatar, of course they need to check what each prop is for. That's why comments are here in propTypes definition
@0xmiroslav I think you might have not understood my comment.
Lets say you are a user of the component Avatar
When looking at the props you would see imageStyles. This means that if i use Avatar and supply it with a imageStyles i should expect that style to be applied at all times.
The fact that its not being applied in some edge cases (this.state.imageError) is bad design on the avator component side.
Both of you have good points. Let's drop the discussion here and wait for the assigned Enginner.
Let's say user wants to set image own style props (i.e. resizeMode) to Image!
After applying your solution, those will be applied to View (Icon wrapper) as well which is wrong