fix wrong height calculation in android modal with statusBarTranslucent #14704

situchan · 2023-01-31T19:12:21Z

Details

Include status bar height in deviceHeight calculation when statusBarTranslucent prop set to true in Modal

Fixed Issues

$ #14563
PROPOSAL: #14563 (comment)

Tests

Login with any account
Go to Setting -> Payments
Tab "Add payment method" button
Verify that no white area appears on "Add Payment method" button

Verify that no errors appear in the JS console

Offline tests

Cannot test in offline mode because "Add Payment method" is disabled

QA Steps

Login with any account
Go to Setting -> Payments
Tab "Add payment method" button
Verify that no white area appears on "Add Payment method" button

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- 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 followed proper code patterns (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 is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- 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 followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
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
I verified that if a function's arguments changed that all usages have also been updated correctly
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 any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
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.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mchrome.mp4

Mobile Web - Safari

msafari.mp4

Desktop

desktop.mov

iOS

ios.mp4

Android

android.mp4

melvin-bot · 2023-01-31T19:12:29Z

@arosiclair @mollfpr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

arosiclair · 2023-02-01T15:13:40Z

src/components/Modal/BaseModal.js

@@ -107,7 +107,7 @@ class BaseModal extends PureComponent {
                hasBackdrop={this.props.fullscreen}
                coverScreen={this.props.fullscreen}
                style={modalStyle}
-                deviceHeight={this.props.windowHeight}
+                deviceHeight={this.props.windowHeight + ((this.props.statusBarTranslucent && StatusBar.currentHeight) || 0)}


Can you just add a small comment on how this fixes android?

// When `statusBarTranslucent` is true on Android, the modal fully covers the status bar. // Since `windowHeight` doesn't include status bar height, it should be added in the `deviceHeight` calculation.

How about this?

Added some small edits to your snippet. Should be good now 👍

arosiclair

LGTM. Checklist is all yours @mollfpr

mollfpr · 2023-02-01T16:21:33Z

@arosiclair Thanks for taking the review quickly! Yup, I'm on it the checklist now!

mollfpr · 2023-02-01T17:01:35Z

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:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- 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 is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- 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 any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
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.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

14704.Web.mov

Mobile Web - Chrome

14704.mWeb.Chrome.MP4

Mobile Web - Safari

14704.mWeb.Safari.mp4

Desktop

14704.Desktop.mov

iOS

14704.iOS.mp4

Android

14704.Android.MP4

mollfpr

Ready to merge @arosiclair

github-actions · 2023-02-02T16:04:13Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	678.534 ms → 692.935 ms (+14.401 ms, +2.1%)
Open Search Page TTI	608.587 ms → 617.638 ms (+9.051 ms, +1.5%)
App start nativeLaunch	19.517 ms → 20.063 ms (+0.545 ms, +2.8%)
App start regularAppStart	0.014 ms → 0.017 ms (+0.003 ms, +19.6%) 🟡
App start runJsBundle	191.219 ms → 181.483 ms (-9.736 ms, -5.1%)

Show details

Name	Duration
App start TTI	Baseline Mean: 678.534 ms Stdev: 43.986 ms (6.5%) Runs: 613.8798529999331 617.8542570001446 626.8649639999494 634.1728659998626 635.825230000075 637.6199799999595 647.7741860002279 649.5745459999889 650.1106759998947 650.8268340001814 650.9834659998305 654.6560689997859 656.888609000016 660.4784849998541 660.9552609999664 661.8282130002044 665.6444049999118 666.9003539998084 671.7323380000889 675.50955099985 690.6314119999297 691.3533660001121 694.7095059999265 718.0400439999066 722.0302479998209 724.7174160000868 725.8482539998367 727.1243610000238 727.2269060001709 735.7636319999583 756.808544000145 808.7503430000506 Current Mean: 692.935 ms Stdev: 33.239 ms (4.8%) Runs: 621.1332290000282 638.124286999926 657.7774950000457 660.0882000001147 661.6749900002033 668.6224030000158 669.395591000095 671.5483249998651 671.6426559998654 672.1904350002296 677.2735910001211 681.9872510000132 684.3609980000183 685.3651569997892 689.6926350002177 689.6943049998954 690.3928570002317 692.0277300002053 692.0531979999505 696.5940069998614 697.3387569999322 698.9637980000116 699.9655570001341 701.8203449998982 717.1165919997729 717.5152719998732 719.6601339997724 724.0603840001859 726.9657560000196 754.5811140001751 759.5949690002017 784.6989079997875
Open Search Page TTI	Baseline Mean: 608.587 ms Stdev: 16.681 ms (2.7%) Runs: 573.1417240002193 578.8799239997752 581.0319830002263 589.8354490003549 590.6553560001776 593.3088790001348 595.5492759998888 596.9324950003065 598.3260909998789 598.7931320001371 601.8244220004417 603.2228599996306 605.0422770003788 606.7290860000066 606.7831220002845 606.9329840000719 607.4385580001399 610.5955000002868 611.3719069999643 611.5491949999705 615.137614000123 615.9484860002995 616.2555749998428 618.1210529999807 622.0711269997992 623.5634360001422 625.0062259999104 626.3021240001544 628.29455600027 630.1490479996428 640.1903480002657 645.8136800001375 Current Mean: 617.638 ms Stdev: 22.196 ms (3.6%) Runs: 570.9720459999517 581.0857339999638 583.6845710002817 588.4403480002657 594.3898109998554 596.5744639998302 597.3959560003132 600.700114000123 600.8989260001108 606.1272789998911 609.7263999995776 610.1667070002295 610.7235930003226 617.4152019997127 617.6250410000794 617.8424889999442 620.7883310001343 621.4294430003501 623.3944089999422 623.8677169997245 626.6867269999348 626.9302579998039 627.9265139997005 629.225626999978 631.0328779998235 632.1437589996494 633.2796229999512 635.2113860002719 648.4573570000939 649.6005460000597 662.6173910000362 668.0701089999638
App start nativeLaunch	Baseline Mean: 19.517 ms Stdev: 1.303 ms (6.7%) Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 22 23 Current Mean: 20.063 ms Stdev: 2.499 ms (12.5%) Runs: 18 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 21 21 22 22 22 22 23 24 25 25 27
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (4.5%) Runs: 0.012614000122994184 0.012777000200003386 0.012980000115931034 0.012980000115931034 0.013101999647915363 0.013142999727278948 0.013225000351667404 0.013346000108867884 0.013346000108867884 0.013509000185877085 0.013549999799579382 0.0138349998742342 0.013915999792516232 0.013956000097095966 0.013956999871879816 0.014038000255823135 0.01407900033518672 0.014119999948889017 0.014119999948889017 0.014119999948889017 0.014159999787807465 0.014241000171750784 0.014403999783098698 0.014404000248759985 0.014444999862462282 0.014527000021189451 0.014851999934762716 0.015056000091135502 Current Mean: 0.017 ms Stdev: 0.001 ms (7.0%) Runs: 0.014728999696671963 0.015015000011771917 0.015137000009417534 0.015300000086426735 0.015422000084072351 0.015461999922990799 0.015543000306934118 0.015544000081717968 0.015583999920636415 0.015747999772429466 0.01590899983420968 0.015950000379234552 0.01607300015166402 0.016112999990582466 0.01615400006994605 0.016315999906510115 0.016439000144600868 0.016480000223964453 0.01652099983766675 0.01688600005581975 0.0168869998306036 0.01700799958780408 0.017009000293910503 0.017048999667167664 0.017171999905258417 0.017577999737113714 0.01798499980941415 0.01839200034737587 0.018432999961078167 0.018962000031024218 0.0194089999422431
App start runJsBundle	Baseline Mean: 191.219 ms Stdev: 32.114 ms (16.8%) Runs: 152 156 161 162 162 164 164 167 171 171 172 173 174 174 175 176 178 178 180 183 187 196 208 210 215 223 233 235 246 248 256 269 Current Mean: 181.483 ms Stdev: 12.229 ms (6.7%) Runs: 163 163 163 166 167 172 172 173 174 175 175 176 179 179 180 182 182 182 184 186 190 192 193 194 195 195 197 204 210

OSBotify · 2023-02-02T16:59:13Z

🚀 Deployed to staging by https://github.com/arosiclair in version: 1.2.64-2 🚀

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

OSBotify · 2023-02-04T00:17:32Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

OSBotify · 2023-02-04T01:55:33Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

OSBotify · 2023-02-04T02:01:40Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

fix wrong height calculation in android modal with statusBarTranslucent

d5f4caa

situchan requested a review from a team as a code owner January 31, 2023 19:12

melvin-bot bot requested review from arosiclair and mollfpr and removed request for a team January 31, 2023 19:12

arosiclair reviewed Feb 1, 2023

View reviewed changes

add comment for setting custom deviceHeight in Modal

69f9341

arosiclair approved these changes Feb 1, 2023

View reviewed changes

mollfpr approved these changes Feb 1, 2023

View reviewed changes

arosiclair merged commit 61eb9ba into Expensify:main Feb 2, 2023

thienlnam mentioned this pull request Feb 2, 2023

Deploy Checklist: New Expensify 2023-02-02 #14771

Closed

30 tasks

melvin-bot bot mentioned this pull request Feb 3, 2023

[HOLD for payment 2023-02-10] [$1000] Android - Payment- White line appears when clicking on "Add Payment method" button #14563

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix wrong height calculation in android modal with statusBarTranslucent #14704

fix wrong height calculation in android modal with statusBarTranslucent #14704

situchan commented Jan 31, 2023

melvin-bot bot commented Jan 31, 2023

arosiclair Feb 1, 2023

situchan Feb 1, 2023 •

edited by arosiclair

Loading

arosiclair Feb 1, 2023

arosiclair left a comment

mollfpr commented Feb 1, 2023

mollfpr commented Feb 1, 2023

mollfpr left a comment

github-actions bot commented Feb 2, 2023

OSBotify commented Feb 2, 2023

OSBotify commented Feb 4, 2023

OSBotify commented Feb 4, 2023

OSBotify commented Feb 4, 2023

fix wrong height calculation in android modal with statusBarTranslucent #14704

fix wrong height calculation in android modal with statusBarTranslucent #14704

Conversation

situchan commented Jan 31, 2023

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 31, 2023

arosiclair Feb 1, 2023

Choose a reason for hiding this comment

situchan Feb 1, 2023 • edited by arosiclair Loading

Choose a reason for hiding this comment

arosiclair Feb 1, 2023

Choose a reason for hiding this comment

arosiclair left a comment

Choose a reason for hiding this comment

mollfpr commented Feb 1, 2023

mollfpr commented Feb 1, 2023

Reviewer Checklist

Screenshots/Videos

mollfpr left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 2, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Feb 2, 2023

OSBotify commented Feb 4, 2023

OSBotify commented Feb 4, 2023

OSBotify commented Feb 4, 2023

situchan Feb 1, 2023 •

edited by arosiclair

Loading