[Feat] Add Image loading indicator #8203
Conversation
MelvinBot
requested review from
mountiny and
rushatgabhane
and removed request for
a team
There was a problem hiding this comment.
@mountiny LGTM! 🎉 Works well on all platforms.
- I verified the correct issue is linked in the ### Fixed Issues section above
- I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- I verified there are no console errors related to changes in this PR
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. “toggleReport” and not “onIconClick”).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct english, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct english and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected) - I verified the UI performance was not affected (the performance is the same than
mainbranch) - If a new component is created I verified that
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- Any internal methods are bound to “this” properly so there are no scoping issues
- Any internal methods bound to “this” are necessary to be bound
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
- If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function
- (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
There was a problem hiding this comment.
@rushatgabhane Thanks! Just asking, did you make sure that this regression we had previously has not been happening again? #7900
You have probably not encountered it, but just want to make sure 😅
|
@mountiny Attaching images on iOS simulator doesn't work for me, so I ran the app on my old iPad. |
|
ios simulator (15.x) does not let me upload too. However, it does in safari. |
|
@mdneyazahmad btw, you're missing recording for web |
|
My bad it was removed by mistake. Just updating it. |
|
Done @rushatgabhane |
There was a problem hiding this comment.
@mdneyazahmad @rushatgabhane Thank you very much for your work on this one.
It is a handy feature, great job! Let's hope nothing will backfire this time 🍀
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
I found one performance related issue in my pr. We can fix this issue by extracting this.state = {
- isLoading: true,
+ isLoading: false,
};
+ this.imageLoadingStart = this.imageLoadingStart.bind(this);
+ this.imageLoadingEnd = this.imageLoadingEnd.bind(this);
}
+ imageLoadingStart() {
+ this.setState({isLoading: true});
+ }
+
+ imageLoadingEnd() {
+ this.setState({isLoading: false});
+ }
- onLoadEnd={() => this.setState({isLoading: false})}
+ onLoadStart={this.imageLoadingStart}
+ onLoadEnd={this.imageLoadingEnd} |
|
@mdneyazahmad I remember giving this a try before, and it didn't work (got a perpetual spinner). |
|
@rushatgabhane Exactly this is the issue and this is due to inline function on |
|
Could you please test with #8203 (comment) and make sure it solves that issue? |
|
@rushatgabhane @mdneyazahmad I have created a revert of this PR just to make sure it does not get to Staging, will you be please able to update #8238 to include these changes? Let's really focus on this so there are no hiccups again, otherwise, we should just get back to the drawing board instead of going back and forth. |
|
This PR has been reverted, let's update the PR you have mentioned 🙇 |
Details
Display an
ActivityIndicatorwhen image is being loaded in the chat.Fixed Issues
$ #7584
Tests
ActivityIndicatorappears when image is being downloaded.PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectionsrc/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNameproperty(i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectionsrc/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNameproperty(i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)QA Steps
ActivityIndicatorappears when image is being downloaded.Screenshots
Web
web.mp4
Mobile Web
mweb.mp4
Desktop
mac.mp4
iOS
ios.mp4
Android
android.mp4