Download icon isn't turning green when clicking to download #9266

mollfpr · 2022-06-01T04:42:27Z

Details

Creating a HOC to hold the logic of button state complete
Updating the ContextMenuItem to use the HOC and remove unused code
Updating the HeaderWithCloseButton component to class-based
Replacing the TouchableOpacity component with Pressable component
Use the HOC on HeaderWithCloseButton and implement it to the download button

Fixed Issues

$ #9124

Tests

Go to https://localhost:8080
Log in with any account and select any chat
Click on any attachment
On Preview page click download icon
Verify that the download icon turns green after click, and turns back to grey after a few seconds.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Go to https://staging.new.expensify.com/
Log in with any account and select any chat
Click on any attachment
On Preview page click download icon
Verify that the download icon turns green after click, and turns back to grey after a few seconds.

Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-06-13.at.21.34.41.mov

Mobile Web

Screen.Recording.2022-06-13.at.21.35.56.mov

Screen.Recording.2022-06-13.at.21.35.36.mov

Desktop

Screen.Recording.2022-06-13.at.21.33.13.mov

iOS

Screen.Recording.2022-06-13.at.21.46.19.mov

Android

Screen.Recording.2022-06-13.at.21.48.00.mov

…Opacity with Pressable, change icon download base on the complete state

src/components/withButtonStateComplete.js

rushatgabhane · 2022-06-02T10:27:49Z

src/components/withButtonStateComplete.js

+};
+
+export default function (WrappedComponent) {
+    class WithButtonStateComplete extends Component {


Let's rename WithButtonStateComplete -> WithDelayToggleButtonState
And the default behaviour of the HOC should be to have a 1800ms delay in toggling the state to make it more obvious what it can do.

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

rushatgabhane

@mollfpr left some minor refactor comments

rushatgabhane · 2022-06-03T18:52:19Z

src/components/withDelayToggleButtonState.js

+        /**
+         * @param {Boolean} [resetAfterDelay=true] Impose delay before toggling state
+         */
+        toggleButtonStateComplete(resetAfterDelay = true) {


@mollfpr please refactor this to do an early return and make it work without setting any default value for resetAfterDelay

rushatgabhane · 2022-06-03T18:55:22Z

src/components/ContextMenuItem.js

-        }
-
-        clearTimeout(this.successResetTimer);
+        this.triggerPressAndUpdateSuccess = this.triggerPressAndUpdateSuccess.bind(this);
    }

    /**
     * Called on button press and mark the run


Called on button press and mark the run

Please update this comment. What is mark the run?
One idea is to remove the comment..

Well as part of our style guides JS docs are required.
But i do agree that the comment here is hard to read. How about?

Method to call parent onPress and toggleDelayButtonState

rushatgabhane · 2022-06-03T19:00:41Z

src/components/withDelayToggleButtonState.js

+
+const withDelayToggleButtonStatePropTypes = {
+    /** A value whether the button state is complete */
+    isButtonStateComplete: PropTypes.bool.isRequired,


NAB: How about this?

isButtonStateComplete -> isDelayButtonStateComplete

Yup, that’s more logical!

rushatgabhane · 2022-06-03T19:01:18Z

src/components/withDelayToggleButtonState.js

+    isButtonStateComplete: PropTypes.bool.isRequired,
+
+    /** A function to call to change the complete state */
+    toggleButtonStateComplete: PropTypes.func.isRequired,


NAB: how about this rename?

toggleButtonStateComplete -> toggleDelayButtonState

mollfpr · 2022-06-04T13:59:18Z

Updated video for web and desktop.

rushatgabhane · 2022-06-06T09:49:46Z

@mollfpr Thanks for the rapid changes! Will review this in the evening (~6 hrs)

rushatgabhane · 2022-06-07T07:08:40Z

I'm confused by withDelayToggleButtonState. We should simplify it a bit.
More soon

rushatgabhane

@mollfpr I noticed that we aren't making use of the delay in context menu. So the HOC isn't needed in ContextMenu.

I've created a patch to rename a few things and removed the use of HOC by ContextMenuItem. Please apply this patch and feel free to make any additional changes. Curious for your thoughts

patch here
git apply path/to/file/patch.txt

rushatgabhane · 2022-06-08T13:09:19Z

src/components/ContextMenuItem.js

-            if (this.props.autoReset) {
-                this.successResetTimer = setTimeout(() => this.setState({success: false}), 1800);
-            }
+            this.props.toggleDelayButtonState(this.props.autoReset);


In context menu, we are never using the delay. So we should just use a state instead of the HOC.

Thanks! I’ll apply the patch and try to take a look at the rest.

@rushatgabhane I just check that DetailsPage -> CommunicationsLink is using ContextMenuItem with props autoReset. Also in your patch, the HOC is deleted but HeaderWithCloseButton is still using the HOC.

Cool, yeah I missed CommunicationsLink.

rushatgabhane · 2022-06-13T12:57:12Z

@mollfpr the download button in attachment view isn't resetting it's state.

Screen.Recording.2022-06-13.at.3.55.06.PM.mov

mollfpr · 2022-06-13T13:54:14Z

@mollfpr the download button in attachment view isn't resetting it's state.

Screen.Recording.2022-06-13.at.3.55.06.PM.mov

That's the expected, same as the download button on ContextMenuItem. We can pass the argument to the function to auto-reset the status or leave it like ContextMenuItem.

Are we need more changes? Since your patch seems not complete yet.

rushatgabhane · 2022-06-13T14:01:07Z

We can pass the argument to the function to auto-reset the status

@mollfpr yes let's reset it's status in the AttachmentView.

Are we need more changes

No, that's all :)

mollfpr · 2022-06-13T14:51:07Z

Updated video!

rushatgabhane

@chiragsalian LGTM!

PR Reviewer Checklist

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 (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
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
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
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
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- 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)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

rushatgabhane · 2022-06-13T15:40:09Z

@mollfpr can you please modify the 5th point in tests to -> Verify that the download icon turns green after click, and turns back to grey after a few seconds.

mollfpr · 2022-06-13T15:42:39Z

Updated test steps!

chiragsalian

Minor comments, rest LGTM and works well.

chiragsalian · 2022-06-13T22:26:01Z

src/components/ContextMenuItem.js

    }

-    /**
-     * Called on button press and mark the run
-     */
    triggerPressAndUpdateSuccess() {


missing JS doc

chiragsalian · 2022-06-13T22:26:53Z

src/components/HeaderWithCloseButton.js

+        this.triggerButtonCompleteAndDownload = this.triggerButtonCompleteAndDownload.bind(this);
+    }
+
+    triggerButtonCompleteAndDownload() {


missing js docs

src/components/withDelayToggleButtonState.js

chiragsalian · 2022-06-14T16:39:01Z

src/components/ContextMenuItem.js

-        }
-
-        clearTimeout(this.successResetTimer);
+        this.triggerPressAndUpdateSuccess = this.triggerPressAndUpdateSuccess.bind(this);
    }

    /**
     * Called on button press and mark the run


Well as part of our style guides JS docs are required.
But i do agree that the comment here is hard to read. How about?

Method to call parent onPress and toggleDelayButtonState

chiragsalian · 2022-06-14T16:42:51Z

src/components/HeaderWithCloseButton.js

+    }
+
+    /**
+     * Called on download button press


JS docs should explain what the method does not when it is called. How about,

Method to trigger parent onDownloadButtonPress to download the file and toggleDelayButtonState to set button state and revert it after sometime.

Thank you for your suggestion. I’ll added it.

chiragsalian

LGTM.

PR Reviewer Checklist

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 (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
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
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
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
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- 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)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

chiragsalian · 2022-06-14T21:21:33Z

Merging since rushat approved it before and the changes are minimal since that review.

OSBotify · 2022-06-14T21:30:54Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-06-16T22:07:04Z

🚀 Deployed to staging by @chiragsalian in version: 1.1.78-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2022-06-28T10:07:12Z

🚀 Deployed to production by @sketchydroide in version: 1.1.78-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mollfpr added 3 commits June 1, 2022 11:27

create HOC for button state complete

665d66c

updating the component to use the HOC and remove unused code

227afbd

use HOC, updating the component to use class based, replace Touchable…

ff213e3

…Opacity with Pressable, change icon download base on the complete state

mollfpr requested a review from a team as a code owner June 1, 2022 04:42

melvin-bot bot requested review from chiragsalian and rushatgabhane and removed request for a team June 1, 2022 04:42

rushatgabhane suggested changes Jun 2, 2022

View reviewed changes

mollfpr and others added 3 commits June 3, 2022 17:17

Update src/components/withButtonStateComplete.js

7fbf845

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

renaming

8c5af69

updating to use new name of HOC

992d302

mollfpr requested a review from rushatgabhane June 3, 2022 14:59

rushatgabhane suggested changes Jun 3, 2022

View reviewed changes

mollfpr added 2 commits June 4, 2022 20:51

renaming the state and the toggle function

356db33

updating the state and the toggle function

66fcab9

mollfpr requested a review from rushatgabhane June 4, 2022 13:59

rushatgabhane suggested changes Jun 8, 2022

View reviewed changes

mollfpr added 2 commits June 13, 2022 21:17

Merge branch 'main' of https://github.com/mollfpr/App into f-9124

4d3f6aa

auto reset on HeaderWithCloseButton

712e85a

mollfpr requested a review from rushatgabhane June 13, 2022 14:50

rushatgabhane previously approved these changes Jun 13, 2022

View reviewed changes

chiragsalian requested changes Jun 13, 2022

View reviewed changes

minor jsdoc

b1844d3

mollfpr dismissed rushatgabhane’s stale review via b1844d3 June 14, 2022 01:11

mollfpr requested a review from chiragsalian June 14, 2022 01:12

chiragsalian requested changes Jun 14, 2022

View reviewed changes

add jsdoc more clearly

20107e2

mollfpr requested a review from chiragsalian June 14, 2022 16:54

chiragsalian approved these changes Jun 14, 2022

View reviewed changes

chiragsalian merged commit d8e4ada into Expensify:main Jun 14, 2022

OSBotify mentioned this pull request Jun 16, 2022

Deploy Checklist: New Expensify 2022-06-16 #9480

Closed

50 tasks

This was referenced Jun 23, 2022

[High]: Chrome+Jaws : Screen reader: Role and label are not defined for "back" control. #8392

Closed

[Medium] Color Contrast: Grey icons against white background fail contrast requirements. #8465

Closed

melvin-bot bot mentioned this pull request Jun 28, 2022

[HOLD for payment 2022-07-05] [$250] Attachment - Download icon isn't turning green when clicking to download in Preview Page #9124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download icon isn't turning green when clicking to download #9266

Download icon isn't turning green when clicking to download #9266

mollfpr commented Jun 1, 2022 •

edited

Loading

rushatgabhane Jun 2, 2022

rushatgabhane left a comment •

edited

Loading

rushatgabhane Jun 3, 2022

rushatgabhane Jun 3, 2022

chiragsalian Jun 14, 2022

rushatgabhane Jun 3, 2022

mollfpr Jun 4, 2022

rushatgabhane Jun 3, 2022

mollfpr commented Jun 4, 2022

rushatgabhane commented Jun 6, 2022

rushatgabhane commented Jun 7, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane Jun 8, 2022

mollfpr Jun 8, 2022

mollfpr Jun 9, 2022

rushatgabhane Jun 13, 2022

rushatgabhane commented Jun 13, 2022

mollfpr commented Jun 13, 2022

rushatgabhane commented Jun 13, 2022

mollfpr commented Jun 13, 2022

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Jun 13, 2022

mollfpr commented Jun 13, 2022

chiragsalian left a comment

chiragsalian Jun 13, 2022

chiragsalian Jun 13, 2022

chiragsalian Jun 14, 2022

chiragsalian Jun 14, 2022

mollfpr Jun 14, 2022

chiragsalian left a comment

chiragsalian commented Jun 14, 2022

OSBotify commented Jun 14, 2022

OSBotify commented Jun 16, 2022

OSBotify commented Jun 28, 2022

Download icon isn't turning green when clicking to download #9266

Download icon isn't turning green when clicking to download #9266

Conversation

mollfpr commented Jun 1, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollfpr commented Jun 4, 2022

rushatgabhane commented Jun 6, 2022

rushatgabhane commented Jun 7, 2022 • edited Loading

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane commented Jun 13, 2022

mollfpr commented Jun 13, 2022

rushatgabhane commented Jun 13, 2022

mollfpr commented Jun 13, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane commented Jun 13, 2022

mollfpr commented Jun 13, 2022

chiragsalian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragsalian left a comment

Choose a reason for hiding this comment

PR Reviewer Checklist

chiragsalian commented Jun 14, 2022

OSBotify commented Jun 14, 2022

OSBotify commented Jun 16, 2022

OSBotify commented Jun 28, 2022

mollfpr commented Jun 1, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Jun 7, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane left a comment •

edited

Loading