refactor navigation modal card styles #12366

aimane-chnaif · 2022-11-02T07:31:30Z

Details

refactor navigation modal card styles
fix regressions on modal animated pages with autoFocus enabled

Fixed Issues

Tests

Login with any account
Go to Settings -> Profile
Focus on the name input field (first or last name)
Scroll the page
Verify that autofill suggestions are hidden

NOTE: Since this is navigation card and animation styles refactor, all screens need to be throughly tested throughout the app in all devices and all platforms.

Verify that no errors appear in the JS console

QA Steps

Login with any account
Go to Settings -> Profile
Focus on the name input field (first or last name)
Scroll the page
Verify that autofill suggestions are hidden

NOTE: Since this is navigation card and animation styles refactor, all screens need to be throughly tested throughout the app in all devices and all platforms.

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Mobile Web - Chrome

mchrome.mp4

Mobile Web - Safari

Desktop

iOS

Android

…enabled

mananjadhav · 2022-11-07T05:59:40Z

@aimane-chnaif Appreciate you putting effort in resolving this, I would've appreciated if we could get screencasts instead of screenshots. From the screenshots, it is difficult to make out the autofill. Will you please be able to do it?

Also do you have a list of the screens impacted by the change? That we can attach in the GH body?

aimane-chnaif · 2022-11-07T12:04:04Z

@mananjadhav this issue is for mWeb (also stated in issue description). That's why I took video only for mWeb.

I couldn't get autofill suggestions displayed for first name, last name in native. I don't think autofill is available for any inputs (except email, password) in native iOS/Android.

On web, the issue is reproducible on chrome (works fine on safari) but I hope it wouldn't be an issue. I haven't found solution yet for web. So far, I found out that the root cause is modal. You can verify this by testing this change (before and after) on Login page:

App/src/libs/Navigation/AppNavigator/PublicScreens.js

Line 14 in 0724337

<RootStack.Navigator>

-   <RootStack.Navigator>
+   <RootStack.Navigator mode="modal">

Here's example popular website with the issue:

scroll.mov

Also do you have a list of the screens impacted by the change?

Most auth screens are impacted. Especially, Search, IOUModal pages (IOUAmount, IOUParticipants, IOUConfirm), Settings, New chat/group/room

mananjadhav · 2022-11-08T23:16:34Z

@aimane-chnaif Thanks for the comment. I'll check. Meanwhile, did you get a chance to look at the PR comments?

aimane-chnaif · 2022-11-09T03:35:32Z

Meanwhile, did you get a chance to look at the PR comments

@mananjadhav which comments do you mean?

mananjadhav

1-2 comments

mananjadhav · 2022-11-07T05:56:58Z

src/styles/StyleUtils.js

        width: isSmallScreenWidth ? '100%' : variables.sideBarWidth,
        backgroundColor: 'transparent',
-        height: '100%',
+        alignSelf: 'flex-end',


I understand why we removed absolute position. Did we also need to remove height: 100% ?

mananjadhav · 2022-11-07T05:57:06Z

src/styles/cardStyles/index.js

        width: isSmallScreenWidth ? screenWidth : variables.sideBarWidth,
-        right: 0,
-        height: '100%',
+        alignSelf: 'flex-end',


here too height?

yes, I explained that in proposal.

mananjadhav · 2022-11-09T03:56:35Z

Meanwhile, did you get a chance to look at the PR comments

@mananjadhav which comments do you mean?

Apologies, I had these commented, but they were pending. I just submitted review and you should see comments.

mananjadhav · 2022-11-11T11:47:13Z

I am going to test this tomorrow. I did basic testing on one page, and it seems to be working (although I had some issue reproducing it on main consistently). Please allow me for the weekend to get this done.

marcochavezf · 2022-11-17T21:10:53Z

Hi @aimane-chnaif, could you fix the conflicting file?

…o fix-11336

aimane-chnaif · 2022-11-18T04:28:53Z

Fixed conflicts. I also suggest to revert this change #12509.
Even without height: 'calc(100vh - 100%)', #12005 is not reproducible from my PR. It works well on Safari 16.1

mananjadhav · 2022-11-19T14:47:12Z

Apologies for the delay here. I took some extra time to reproduce the error, as well as testing all the screens.

@marcochavezf All yours.

🎀 👀 🎀
C+ reviewed

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
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 included screenshots or videos 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
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
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 adding the Waiting for Copy label for a copy review 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 other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
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 */
- 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

web-autosuggestion.mov

web-autosuggestion-chrome.mov

Mobile Web - Chrome

Important

mweb-chrome-autosuggestion.mov

Mobile Web - Safari

Desktop

iOS

Android

marcochavezf · 2022-11-21T20:06:19Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
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 included screenshots or videos 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
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
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 adding the Waiting for Copy label for a copy review 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 other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
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 */
- 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

marcochavezf · 2022-11-21T20:08:22Z

Oh seems the GH action that checks the Reviewer Checklist looks for comments that begin with ## Reviewer Checklist 🤔

marcochavezf

LGTM, thank you for addressing this bug guys!

OSBotify · 2022-11-21T20:14:10Z

✋ 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-11-22T16:36:08Z

🚀 Deployed to staging by @marcochavezf in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-22T17:45:53Z

🚀 Deployed to staging by @marcochavezf in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-24T00:04:15Z

🚀 Deployed to production by @luacmartins in version: 1.2.30-0 🚀

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

aimane-chnaif added 2 commits November 2, 2022 15:58

refactor navigation modal card styles

5779f20

fix animation issues while navigating to modal screen with autoFocus …

a6e76f2

…enabled

aimane-chnaif requested a review from a team as a code owner November 2, 2022 07:31

melvin-bot bot requested review from mananjadhav and marcochavezf and removed request for a team November 2, 2022 07:31

aimane-chnaif mentioned this pull request Nov 2, 2022

[$500] mWeb/Chrome - Profile setting - The autofill field remains when scrolling the page #11336

Closed

mananjadhav reviewed Nov 9, 2022

View reviewed changes

Merge branch 'main' of https://github.com/aimane-chnaif/Expensify int…

d2a6919

…o fix-11336

mananjadhav approved these changes Nov 19, 2022

View reviewed changes

marcochavezf approved these changes Nov 21, 2022

View reviewed changes

marcochavezf merged commit 5a03b50 into Expensify:main Nov 21, 2022

hellohublot mentioned this pull request Nov 22, 2022

[$4000] [iOS] - Page freezes when creating New Group and selecting 8 members #9809

Closed

OSBotify mentioned this pull request Nov 22, 2022

Deploy Checklist: New Expensify 2022-11-22 #12934

Closed

40 tasks

Puneet-here mentioned this pull request Nov 24, 2022

[HOLD for payment 2022-12-05] mWeb/Chrome - Split - Next button is missing when you split bill with participants #13011

Closed

Puneet-here mentioned this pull request Nov 24, 2022

[HOLD for payment 2022-12-05] [Performance] Mweb/safari - User can't tap participants in split bill on High Traffic Account. #13008

Closed

luacmartins mentioned this pull request Nov 24, 2022

Revert "refactor navigation modal card styles" #13014

Merged

95 tasks

sakluger mentioned this pull request Nov 28, 2022

[HOLD for payment 2022-11-24] [$250] Bug: Safari- RHN leaves empty space at the bottom after closing the 2nd tab reported by @Tushu17 #12005

Closed

luacmartins mentioned this pull request Nov 29, 2022

[HOLD for payment 2022-12-05] mWeb- The Payment option is hidden after selecting one of the payment option #13009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor navigation modal card styles #12366

refactor navigation modal card styles #12366

aimane-chnaif commented Nov 2, 2022

mananjadhav commented Nov 7, 2022

aimane-chnaif commented Nov 7, 2022

mananjadhav commented Nov 8, 2022

aimane-chnaif commented Nov 9, 2022

mananjadhav left a comment

mananjadhav Nov 7, 2022

mananjadhav Nov 7, 2022

aimane-chnaif Nov 9, 2022

mananjadhav commented Nov 9, 2022 •

edited

Loading

mananjadhav commented Nov 11, 2022

marcochavezf commented Nov 17, 2022

aimane-chnaif commented Nov 18, 2022

mananjadhav commented Nov 19, 2022 •

edited

Loading

marcochavezf commented Nov 21, 2022

marcochavezf commented Nov 21, 2022

marcochavezf left a comment

OSBotify commented Nov 21, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 24, 2022

refactor navigation modal card styles #12366

refactor navigation modal card styles #12366

Conversation

aimane-chnaif commented Nov 2, 2022

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

mananjadhav commented Nov 7, 2022

aimane-chnaif commented Nov 7, 2022

mananjadhav commented Nov 8, 2022

aimane-chnaif commented Nov 9, 2022

mananjadhav left a comment

Choose a reason for hiding this comment

mananjadhav Nov 7, 2022

Choose a reason for hiding this comment

mananjadhav Nov 7, 2022

Choose a reason for hiding this comment

aimane-chnaif Nov 9, 2022

Choose a reason for hiding this comment

mananjadhav commented Nov 9, 2022 • edited Loading

mananjadhav commented Nov 11, 2022

marcochavezf commented Nov 17, 2022

aimane-chnaif commented Nov 18, 2022

mananjadhav commented Nov 19, 2022 • edited Loading

Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

marcochavezf commented Nov 21, 2022

Reviewer Checklist

marcochavezf commented Nov 21, 2022

marcochavezf left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 21, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 24, 2022

mananjadhav commented Nov 9, 2022 •

edited

Loading

mananjadhav commented Nov 19, 2022 •

edited

Loading