Skip to content
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

Fix missing avatars #2099

Merged
merged 5 commits into from
Mar 29, 2021
Merged

Fix missing avatars #2099

merged 5 commits into from
Mar 29, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Mar 26, 2021

Details

Fixed Issues

https://github.com/Expensify/Expensify/issues/154715

Tests

  1. Test loading images against production API
  2. Scroll to bottom of "Chats" list and make sure all avatars appear
  3. Pop in and out of various chats and verify avatars appear and any attachments load

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Mar 26, 2021
@marcaaron marcaaron requested a review from a team as a code owner March 26, 2021 03:27
@botify botify requested review from Dal-Papa and removed request for a team March 26, 2021 03:27
@marcaaron marcaaron requested review from a team and removed request for Dal-Papa March 26, 2021 03:27
@botify botify requested review from Dal-Papa and removed request for a team March 26, 2021 03:28

// Setting removeClippedSubviews will break text selection on Android
// eslint-disable-next-line react/jsx-props-no-multi-spaces
removeClippedSubviews={false}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrewGable do you remember how this broke text selection on Android? I tested it and seemed to work ok to me, but maybe I misinterpreted this comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it was something along the lines of https://stackoverflow.com/a/66703331/2509962

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted with Andrew and neither one of us could remember why it was added. We're also going to implement a long press menu to select the entire contents of a message IIRC so whatever this refers to (still not entirely sure) might be even less relevant in the future.

src/components/InvertedFlatList/BaseInvertedFlatList.js Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
import {SectionList} from 'react-native';

export default SectionList;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this file do ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much 😄 . It imports the unmodified SectionList and then re-exports it. This is because we have different builds depending on the platform and will switch extensions e.g.

import SectionList from './SectionList';

On web, this will pull from /SectionList/index.js but on Android it will use /SectionList/index.android.js

So we are basically saying, when building for web use the unmodified SectionList

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well for me, my only question before merging is the same as #2099 (comment) to confirm there isn't a regression we're unaware of. To clarify, I don't see any text selection regressions while testing, but I want to confirm I'm not missing anything.

@marcaaron
Copy link
Contributor Author

Gonna proceed with merging this one since:

  1. We're not sure exactly what we were trying to avoid in Remove text selection hack for Android as FlatList fixes everything! #578
  2. I'm not seeing any negative effects related to text selection on android
  3. We are implementing a "Copy to Clipboard" feature on long press anyway to help with copy/pasting text on native

@marcaaron marcaaron merged commit ac6ab0a into master Mar 29, 2021
@marcaaron marcaaron deleted the marcaaron-greyAvatarFix branch March 29, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants