Skip to content

[HOLD for payment 2022-04-13] When clicking on chat open Image viewer. It does not show a loading indicator - reported by @mdneyazahmad #7905

Closed

Description

@mvtglobally

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open any chat
  2. Send bigger size image
  3. Open image

Expected Result:

There should be loading indicator while image is opening

Actual Result:

There is no loading indicator while image is opening

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
There are 2 issues logged before which may be related or fix this issue.
#7584
#7463

Screen.Recording.2022-02-06.at.4.42.18.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @mdneyazahmad
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644146117048899

View all open jobs on GitHub

Activity

  1. MelvinBot commented on Feb 25, 2022

    @MelvinBot
    Contributor

    Triggered auto assignment to @sakluger (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

  2. removed
    AutoAssignerTriageAuto assign issues for triage to an available triage team member
    on Feb 25, 2022
  3. mdneyazahmad commented on Feb 25, 2022

    @mdneyazahmad
    Contributor

    Edit: Please, ignore this proposal, updated here #7905 (comment)

    Proposal

    Proposing early as I reported this issue

    Create a flag isLoading to store the state of the loading action. Add onLoadEnd listener on Image and when fired set the isLoading to false.

    Conditionally render ActivityIndicator. Applying styles.dNode because the Image component has to be mounted on the dom in order to load and fire the onLoadEnd event on Image.

    diff --git a/src/components/ImageView/index.js b/src/components/ImageView/index.js
    index 96a191e46..4996f8d7b 100644
    --- a/src/components/ImageView/index.js
    +++ b/src/components/ImageView/index.js
    @@ -1,12 +1,13 @@
     import React, {PureComponent} from 'react';
     import PropTypes from 'prop-types';
     import {
    -    View, Image, Pressable,
    +    View, Image, Pressable, ActivityIndicator,
     } from 'react-native';
     import styles from '../../styles/styles';
     import * as StyleUtils from '../../styles/StyleUtils';
     import canUseTouchScreen from '../../libs/canUseTouchscreen';
     import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';
    +import themeColors from '../../styles/themes/default';
     
     const propTypes = {
         /** URL to full-sized image */
    @@ -20,6 +21,7 @@ class ImageView extends PureComponent {
             this.scrollableRef = null;
             this.canUseTouchScreen = canUseTouchScreen();
             this.state = {
    +            isLoading: true,
                 containerHeight: 0,
                 containerWidth: 0,
                 isZoomed: false,
    @@ -188,14 +190,25 @@ class ImageView extends PureComponent {
                         styles.pRelative,
                     ]}
                 >
    +                {this.state.isLoading && (
    +                    <ActivityIndicator
    +                        size="large"
    +                        style={[styles.flex1]}
    +                        color={themeColors.textSupporting}
    +                    />
    +                )}
    +
                     <Pressable
    -                    style={{
    -                        ...StyleUtils.getZoomSizingStyle(this.state.isZoomed, this.state.imgWidth, this.state.imgHeight, this.state.zoomScale,
    -                            this.state.containerHeight, this.state.containerWidth),
    -                        ...StyleUtils.getZoomCursorStyle(this.state.isZoomed, this.state.isDragging),
    -                        ...this.state.isZoomed && this.state.zoomScale > 1 ? styles.pRelative : styles.pAbsolute,
    -                        ...styles.flex1,
    -                    }}
    +                    style={[
    +                        {
    +                            ...StyleUtils.getZoomSizingStyle(this.state.isZoomed, this.state.imgWidth, this.state.imgHeight, this.state.zoomScale,
    +                                this.state.containerHeight, this.state.containerWidth),
    +                            ...StyleUtils.getZoomCursorStyle(this.state.isZoomed, this.state.isDragging),
    +                            ...this.state.isZoomed && this.state.zoomScale > 1 ? styles.pRelative : styles.pAbsolute,
    +                            ...styles.flex1,
    +                        },
    +                        this.state.isLoading && styles.dNone,
    +                    ]}
                         onPressIn={(e) => {
                             const {pageX, pageY} = e.nativeEvent;
                             this.setState({
    @@ -238,6 +251,7 @@ class ImageView extends PureComponent {
                                 styles.w100,
                             ]}
                             resizeMode={this.state.isZoomed ? 'contain' : 'center'}
    +                        onLoadEnd={() => this.setState({isLoading: false})}
                         />
                     </Pressable>
                 </View>
    

    Result

    loading-indicator.mp4
  4. MelvinBot commented on Feb 28, 2022

    @MelvinBot
    Contributor

    @sakluger Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

  5. sakluger commented on Mar 2, 2022

    @sakluger
    Contributor

    Thanks for reporting this! Seems like something that we should fix, passing along to engineering to triage the proposal.

  6. removed their assignment
    on Mar 2, 2022
  7. MelvinBot commented on Mar 2, 2022

    @MelvinBot
    Contributor

    Triggered auto assignment to @nkuoch (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

  8. 48 remaining items

  9. mdneyazahmad commented on Mar 21, 2022

    @mdneyazahmad
    Contributor

    @rushatgabhane We are not showing the thumbnail images from chat. Once we open the modal it downloads and only when we open it second time it shows the thumbnail.

  10. mountiny commented on Mar 21, 2022

    @mountiny
    Contributor

    @mdneyazahmad @rushatgabhane

    I think it makes sense to show the thumbnail instead of nothing in case the enlarged thumbnail quality won't be too bad. If you know what I mean. For phone or smaller screens where the max size of the image shown is actually not much larger than the thumbnail in the chat, this could certainly be a way to go, however, we would need to see how the enlarged thumbnail looks on large desktop.

    If it would be too blurry I think the effect on UX would be bad instead of beneficial.

    It could be worth exploring.

  11. mdneyazahmad commented on Mar 21, 2022

    @mdneyazahmad
    Contributor

    I too agree with showing thumbnail on mobile and also on desktop. May be, we start with thumbnail and show a loading indicator on top of it. We can prevent it stretching to whole screen on desktop, at least we show something.

  12. mountiny commented on Mar 21, 2022

    @mountiny
    Contributor

    We can prevent it stretching to whole screen on desktop, at least we show something

    I don't think showing a smaller picture then jumping to a larger one once loaded would be nice. Either we would stretch the thumbnail to the full size and add a spinner over it and then just swap the images once loaded or have nothing as we have now in case the quality of the thumbnail stretched would be too bad.

    However, we can focus on that later on. Let's get what we have now right with no regressions and we can then explore how it will look like with the thumbnail and maybe have some poll in Slack to decide what people think is better. But let's keep this one in the boundaries of what has already been planned 👍

  13. parasharrajat commented on Mar 21, 2022

    @parasharrajat
    Member

    I agree with Mountiny #7905 (comment). Let's get the loader rolling first.

  14. added
    Awaiting PaymentAuto-added when associated PR is deployed to production
    and removed on Apr 6, 2022
  15. changed the title [-]When clicking on chat open Image viewer. It does not show a loading indicator - reported by @mdneyazahmad[/-] [+][HOLD for payment 2022-04-13] When clicking on chat open Image viewer. It does not show a loading indicator - reported by @mdneyazahmad[/+] on Apr 6, 2022
  16. melvin-bot commented on Apr 6, 2022

    @melvin-bot

    The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.51-0 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 2022-04-13. 🎊

  17. kadiealexander commented on Apr 13, 2022

    @kadiealexander
    Contributor

    Paid @parasharrajat for C+ and @mdneyazahmad for the fix (+ reporting bonus). Thanks for your work team!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Awaiting PaymentAuto-added when associated PR is deployed to productionDailyKSv2EngineeringExternalAdded to denote the issue can be worked on by a contributorImprovementItem broken or needs improvement.ReviewingHas a PR in review

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions