Fixed : One of the popup modal options is highlighted by default #8581

metehanozyurt · 2022-04-10T06:21:39Z

Details

One of the popup modal options is highlighted by default, issue fixed

Fixed Issues

Tests

1-) Open Expensify App.
2-) Go to any user chat click on ➕ ,
3-) Go to any user chat click call button (up right on screen) ,
4-) When two users online request money to another one and click pay button and click arrow button like in this picture below,
5-) Go to Settings -> Profile -> Click on camera icon
6-) Go to Settings -> Workspace -> General Settings -> Click on camera icon to change picture
7-) Go to Settings -> Workspace -> Click tree dots button (up right on screen)
8-) Should not any item highlighted when open popover menu

pay button:

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

1-) Open Expensify App.
2-) Go to any user chat click on ➕ ,
3-) Go to any user chat click call button (up right on screen) ,
4-) When two users online request money to another one and click pay button and click arrow button like in this picture below,
5-) Go to Settings -> Profile -> Click on camera icon
6-) Go to Settings -> Workspace -> General Settings -> Click on camera icon to change picture
7-) Go to Settings -> Workspace -> Click tree dots button (up right on screen)
8-) Should not any item highlighted when open popover menu

pay button:

Verify that no errors appear in the JS console

Screenshots

Web

web-2022-04-30.mp4

Mobile Web

ios-web-2022-04-30.mp4

android-Web-2022-04-30.mp4

Desktop

desktop-2022-04-30.mp4

iOS

ios-Native-2022-04-30.mp4

Android

android-native-2022-04-30.mp4

Santhosh-Sellavel · 2022-04-10T06:41:12Z

@metehanozyurt Please add video recordings.

metehanozyurt · 2022-04-10T06:52:51Z

Of course , I do it immediately . I would have a few questions.
I added it as a test video for the web version. Would it be enough for me to shoot videos for web and desktop, would you like me to shoot for mobile versions as well? I can also do it in slow motion if you want @Santhosh-Sellavel .

metehanozyurt · 2022-04-10T07:28:24Z

add video records all platform and for web and desktop make slow motion records too. Thank you @Santhosh-Sellavel for guiding me with your comments 🙏.

Santhosh-Sellavel · 2022-04-11T15:30:19Z

@metehanozyurt
Add QA Steps, also include all the areas (Chat, Profile Picture change, Workspace Image change, Workspace more etc) where we should verify changes. if possible update screen recordings to cover all these.

metehanozyurt · 2022-04-12T06:28:52Z

I upload new videos and wrote all steps @Santhosh-Sellavel thank you 🙏 .
I think i have one issue i validate on stage version too. I am writing here because it is an important detail that you will encounter while checking. This issue like this .
Sometimes this button not look like this

fullscreen versions :

when user online request money comes :

logout and login

I would like to report this situation on the Slack channel, if you have permission. Thank you.

Santhosh-Sellavel · 2022-04-12T13:23:36Z

@metehanozyurt you can report it on slack, if you are not on slack.

To request an invite to the channel, just email contributors@expensify.com with the subject Slack Channel Invite and include a link to your Upwork profile.

metehanozyurt · 2022-04-12T16:37:11Z

Thank you @Santhosh-Sellavel . I also tested this button, you can see it in the videos. Kind regards.

Santhosh-Sellavel · 2022-04-12T20:05:31Z

@metehanozyurt
Thanks for all your efforts. Reviewing this now.

Santhosh-Sellavel · 2022-04-12T20:13:20Z

@metehanozyurt
This is still broken:

Screen.Recording.2022-04-13.at.1.42.40.AM.mov

From PR attachments:

metehanozyurt · 2022-04-12T20:23:12Z

@Santhosh-Sellavel i am so sorry . I guess my eyes couldn't distinguish because these buttons are colored. I'm looking into the cause right now.

metehanozyurt · 2022-04-12T20:34:37Z

this file cause problem for call icons.
Popover/index.js can i change this file.

App/src/components/Popover/index.js

Lines 36 to 37 in 1814323

    
           animationIn={props.isSmallScreenWidth ? undefined : props.animationIn} 
        
           animationOut={props.isSmallScreenWidth ? undefined : props.animationOut}

to like this

            animationIn={props.isSmallScreenWidth ? undefined : 'fadeIn'}
            animationOut={props.isSmallScreenWidth ? undefined : 'fadeOut'}

I would like to research if any other components uses it before I change it, if you see fit.

Santhosh-Sellavel · 2022-04-12T20:40:08Z

I would like to research if any other components uses it before I change it, if you see fit.

Yes, please check!

metehanozyurt · 2022-04-13T16:53:37Z

I done my research. my results is as follows. Popover/index.js used by many components.

Popover/index.js uses by this components

animationIn	animationOut
no	no	BasePaymentsPage.js
...props	...props	BasePopoverMenu.js
yes	yes	VideoChatButtonAndMenu.js
...props	...props	PopoverWithMeasuredContent.js
no	no	PasswordPopover.js
no	no	ConfirmPopover.js

PopoverWithMeasuredContent.js uses by this components

animationIn	animationOut
no	no	EmojiPicker/index.js
yes(fadeIn)	no	PopoverReportActionContextMenu.js

BasePopoverMenu.js uses by this components

animationIn	animationOut
yes(fadeIn)	yes(fadeOut)	PopoverMenu/index.js (on my commits)
...props	...props	PopoverMenu/index.native.js

Conclusion

I think it will increase the complexity of the sample code I suggested above. Instead, I think we should give the fadeIn and fadeOut parameters on the VideoChatButtonAndMenu.js file.

In the code fragments below, you can see that these parameters are not set on small screens.

App/src/components/Popover/index.js

Lines 36 to 37 in 1814323

    
           animationIn={props.isSmallScreenWidth ? undefined : props.animationIn} 
        
           animationOut={props.isSmallScreenWidth ? undefined : props.animationOut}

And

App/src/components/Popover/index.js

Lines 20 to 21 in 1814323

    
           animationIn={props.isSmallScreenWidth ? undefined : props.animationIn} 
        
           animationOut={props.isSmallScreenWidth ? undefined : props.animationOut}

I had to expand the solution method that I mentioned in my proposal. Because this was an issue I encountered while shooting screen videos.

Santhosh-Sellavel · 2022-04-18T18:51:35Z

I think it will increase the complexity of the #8581 (comment) suggested above. Instead, I think we should give the fadeIn and fadeOut parameters on the VideoChatButtonAndMenu.js file.

I am fine with both the above solution or this solution

I think it will increase the complexity

@metehanozyurt What's the complexity here?

metehanozyurt · 2022-04-20T04:52:36Z

That's why I think of it as complexity. It is already ignored on small screens. We also set it for big screens. That's why I was wondering if it should be given directly on the page. I was worried that the developer who would look at the code next to me would think that it would make more sense to change it on the page.

According to the decision of you managers, I can act in that way @Santhosh-Sellavel 🙏 . I am very happy with what I have learned from you 🙏 .

metehanozyurt · 2022-04-21T05:36:47Z

I made a change on the page for the reason I mentioned above. If you don't see fit, I'd be happy to change it. Thank you @Santhosh-Sellavel .

Video records all platforms after this changed:
Desktop-Web:

call-button-web-desktop.mp4

ios native and mWeb:

ios-and-ios-mweb.mp4

android native and mWeb:

call-button-android-mweb.mp4

Santhosh-Sellavel · 2022-04-22T21:12:21Z

@roryabraham

What's your thought on this #8581 (comment)

roryabraham · 2022-04-26T01:20:12Z

What's your thought on this #8581 (comment)

Well, I'm not sure the context. Why do we need to add a default animation? Can we just pass the animationIn and animationOut props in the instances where we need to?

If it makes sense to add these default animations, we should inject them as default props rather than inline in the component.

Santhosh-Sellavel · 2022-04-27T17:14:42Z

If it makes sense to add these default animations, we should inject them as default props rather than inline in the component.

@metehanozyurt Can we do this?

metehanozyurt · 2022-04-27T18:57:16Z

Of course i will do it like this way @Santhosh-Sellavel . I just want to make sure I understand correctly. I don't want to make angry anyone.
I can commit immediately.

I will remove these lines:

App/src/components/ThreeDotsMenu/index.js

Lines 97 to 98 in ff3625f

    
           animationIn="fadeInDown" 
        
           animationOut="fadeOutUp"

App/src/components/VideoChatButtonAndMenu.js

Lines 135 to 136 in 0d1ec3d

    
           animationIn="fadeInDown" 
        
           animationOut="fadeOutUp"

App/src/components/AvatarWithImagePicker.js

Lines 223 to 224 in 2b7d6d0

    
           animationIn="fadeInDown" 
        
           animationOut="fadeOutUp"

App/src/components/ButtonWithMenu.js

Lines 85 to 86 in 6803d00

    
           animationIn="fadeInUp" 
        
           animationOut="fadeOutDown"

App/src/pages/home/report/ReportActionCompose.js

Lines 452 to 453 in 16b130b

    
           animationIn="fadeInUp" 
        
           animationOut="fadeOutDown"

App/src/components/Popover/index.js

Lines 20 to 21 in 1814323

    
           animationIn={props.isSmallScreenWidth ? undefined : props.animationIn} 
        
           animationOut={props.isSmallScreenWidth ? undefined : props.animationOut}

App/src/components/Popover/index.js

Lines 36 to 37 in 1814323

    
           animationIn={props.isSmallScreenWidth ? undefined : props.animationIn} 
        
           animationOut={props.isSmallScreenWidth ? undefined : props.animationOut}

This one stay like this revert my change:

App/src/components/PopoverMenu/index.js

Line 23 in 1814323

return <BasePopoverMenu {...props} onItemSelected={selectItem} />;

Already fadeIn and fadeOut default props in here so i will not do anything this for this file:

App/src/components/PopoverMenu/popoverMenuPropTypes.js

Lines 55 to 56 in 1814323

    
           animationIn: 'fadeIn', 
        
           animationOut: 'fadeOut',

I will change this file like this:

App/src/components/Popover/popoverPropTypes.js

Lines 17 to 22 in 3a58a6e

    
           const defaultProps = { 
        
               ...(_.omit(defaultModalProps, ['type', 'popoverAnchorPosition'])), 
        
               // Anchor position is optional only because it is not relevant on mobile 
        
               anchorPosition: {}, 
        
           };

const defaultProps = {
    ...(_.omit(defaultModalProps, ['type', 'popoverAnchorPosition'])),

    animationIn: 'fadeIn',
    animationOut: 'fadeOut',

    // Anchor position is optional only because it is not relevant on mobile
    anchorPosition: {},
    disableAnimation: true,
};

Santhosh-Sellavel · 2022-04-27T19:39:11Z

@metehanozyurt Looks good, please verify that it doesn’t break any existing behavior, Thanks!

Santhosh-Sellavel · 2022-04-29T20:33:03Z

@metehanozyurt Test's well. update the video in the PR.

Santhosh-Sellavel · 2022-04-29T20:58:54Z

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.

metehanozyurt · 2022-04-30T08:34:33Z

Updated all platform videos on PR @Santhosh-Sellavel . Thanks.

Santhosh-Sellavel

LGTM Tests well!

@roryabraham

OSBotify · 2022-05-03T19:22:55Z

✋ 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-05-09T20:03:27Z

🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-05-13T16:12:23Z

🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀

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

added fadeIn value to animationIn props according to issue 8133

09d1942

metehanozyurt requested a review from a team as a code owner April 10, 2022 06:21

melvin-bot bot requested review from roryabraham and Santhosh-Sellavel and removed request for a team April 10, 2022 06:21

added fadeOut value to animationOut props

47d82d6

videoChatButtonAndMenu changed to fadeIn and fadeOut

ddda2e1

removed animationIn and animationOut in pages , added default props

41b6912

Santhosh-Sellavel approved these changes Apr 30, 2022

View reviewed changes

Merge branch 'Expensify:main' into issue-8133

6f52136

roryabraham approved these changes May 3, 2022

View reviewed changes

roryabraham merged commit 3fc95c6 into Expensify:main May 3, 2022

OSBotify mentioned this pull request May 10, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed : One of the popup modal options is highlighted by default #8581

Fixed : One of the popup modal options is highlighted by default #8581

metehanozyurt commented Apr 10, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 10, 2022

metehanozyurt commented Apr 10, 2022

metehanozyurt commented Apr 10, 2022

Santhosh-Sellavel commented Apr 11, 2022

metehanozyurt commented Apr 12, 2022

Santhosh-Sellavel commented Apr 12, 2022 •

edited

Loading

metehanozyurt commented Apr 12, 2022

Santhosh-Sellavel commented Apr 12, 2022

Santhosh-Sellavel commented Apr 12, 2022 •

edited

Loading

metehanozyurt commented Apr 12, 2022

metehanozyurt commented Apr 12, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 12, 2022 •

edited

Loading

metehanozyurt commented Apr 13, 2022

Santhosh-Sellavel commented Apr 18, 2022 •

edited

Loading

metehanozyurt commented Apr 20, 2022

metehanozyurt commented Apr 21, 2022

Santhosh-Sellavel commented Apr 22, 2022

roryabraham commented Apr 26, 2022

Santhosh-Sellavel commented Apr 27, 2022 •

edited

Loading

metehanozyurt commented Apr 27, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 27, 2022

Santhosh-Sellavel commented Apr 29, 2022

Santhosh-Sellavel commented Apr 29, 2022 •

edited

Loading

metehanozyurt commented Apr 30, 2022

Santhosh-Sellavel left a comment

OSBotify commented May 3, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

Fixed : One of the popup modal options is highlighted by default #8581

Fixed : One of the popup modal options is highlighted by default #8581

Conversation

metehanozyurt commented Apr 10, 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

Santhosh-Sellavel commented Apr 10, 2022

metehanozyurt commented Apr 10, 2022

metehanozyurt commented Apr 10, 2022

Santhosh-Sellavel commented Apr 11, 2022

metehanozyurt commented Apr 12, 2022

Santhosh-Sellavel commented Apr 12, 2022 • edited Loading

metehanozyurt commented Apr 12, 2022

Santhosh-Sellavel commented Apr 12, 2022

Santhosh-Sellavel commented Apr 12, 2022 • edited Loading

metehanozyurt commented Apr 12, 2022

metehanozyurt commented Apr 12, 2022 • edited Loading

Santhosh-Sellavel commented Apr 12, 2022 • edited Loading

metehanozyurt commented Apr 13, 2022

Conclusion

Santhosh-Sellavel commented Apr 18, 2022 • edited Loading

metehanozyurt commented Apr 20, 2022

metehanozyurt commented Apr 21, 2022

Santhosh-Sellavel commented Apr 22, 2022

roryabraham commented Apr 26, 2022

Santhosh-Sellavel commented Apr 27, 2022 • edited Loading

metehanozyurt commented Apr 27, 2022 • edited Loading

Santhosh-Sellavel commented Apr 27, 2022

Santhosh-Sellavel commented Apr 29, 2022

Santhosh-Sellavel commented Apr 29, 2022 • edited Loading

PR Reviewer Checklist

metehanozyurt commented Apr 30, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

OSBotify commented May 3, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

metehanozyurt commented Apr 10, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 12, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 12, 2022 •

edited

Loading

metehanozyurt commented Apr 12, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 12, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 18, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 27, 2022 •

edited

Loading

metehanozyurt commented Apr 27, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 29, 2022 •

edited

Loading