[IS-2151] fixed FAB issue #2305

aliabbasmalik8 · 2021-04-08T19:40:41Z

Details

To fix FAB position on iPad I used keyboardavoidingview for IOS Platform.

Fixed Issues

Fixes https://github.com//issues/2151

Tests

Test on all platform and FAB Icon is shown on the appropriate position and tested with click on FAB ICON and it's working on all platform

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iPad

Android

Luke9389 · 2021-04-08T22:01:16Z

Hey @aliabbasmalik8, looks like our linter is pointing out some changes that need to be made. I'll get pinged once you fix those, and then I'll take a look

aliabbasmalik8 · 2021-04-08T22:29:14Z

Hi @Luke9389 lint issues fixed, please have a look

Hey @aliabbasmalik8, looks like our linter is pointing out some changes that need to be made. I'll get pinged once you fix those, and then I'll take a look

Luke9389 · 2021-04-08T22:52:46Z

Hello again @aliabbasmalik8, thanks for that fast turn-around. Something just came up on my end, so I'll have to review this tomorrow.

Luke9389 · 2021-04-09T19:43:11Z

Hello again @aliabbasmalik8, would you please add screenshots for web and mobile web? I see that you've checked the boxes that you tested on those platforms, but screenshots are also required.

Luke9389 · 2021-04-09T19:51:07Z

src/components/Fab/index.ios.js

+
+function Fab({onPress, isActive}) {
+    return (
+        <KeyboardAvoidingView behavior="position">


In our documentation, in the Cross Platform 99.999% section, we strongly discourage platform specific code. I understand that the problem is only happening on iOS but this does not mean our solution needs to be iOS specific. Does KeyboardAvoidingView only work on iOS? If not, then we don't need this ios specific file.

@Luke9389 for android we have no need for KeyboardAvoidingView and we need another configuration for KeyboardAvoidingView if we want to add so that's the reason I moved to platform-specific code.

Luke9389 · 2021-04-09T19:51:49Z

src/pages/home/sidebar/SidebarScreen.js

@@ -3,7 +3,7 @@ import {View} from 'react-native';
 import styles from '../../../styles/styles';
 import SidebarLinks from './SidebarLinks';
 import CreateMenu from '../../../components/CreateMenu';
-import FAB from '../../../components/FAB';
+import FAB from '../../../components/Fab';


I think our preference is to keep FAB as an all caps name to indicate that it's an acronym (Floating Action Button)

Updated accordingly.

aliabbasmalik8 · 2021-04-09T20:34:35Z

Hello again @aliabbasmalik8, would you please add screenshots for web and mobile web? I see that you've checked the boxes that you tested on those platforms, but screenshots are also required.

Added screenshots

src/components/FAB/FAB.js

marcaaron · 2021-04-13T23:00:58Z

src/components/FAB/index.js

+
+function Fab({onPress, isActive}) {
+    return <FAB onPress={onPress} isActive={isActive} />;
+}


I'm not following why we can't use the KeyboardAvoidingView everywhere? What is the problem exactly?

For android we have no need to use KeyboardAvoidingView

If we want to add KeyboardAvoidingView for android then we need to make many style changes(for both big and small screen) on the SidebarScreen because FAB onPress not working with KeyboardAvoidingView in android. (can fix by making style changes but it's no need so that's the reason not did, will do on demand)

Alright, I'll take your word for it. Although I'm not sure which style changes you mean in this case.

We should add some kind of comment so that we will understand what purpose splitting these files in two serves?

marcaaron · 2021-04-14T00:24:52Z

src/components/FAB/index.js

+
+function Fab({onPress, isActive}) {
+    return <FAB onPress={onPress} isActive={isActive} />;
+}


Alright, I'll take your word for it. Although I'm not sure which style changes you mean in this case.

marcaaron · 2021-04-14T00:25:56Z

src/components/FAB/index.js

+
+function Fab({onPress, isActive}) {
+    return <FAB onPress={onPress} isActive={isActive} />;
+}


We should add some kind of comment so that we will understand what purpose splitting these files in two serves?

src/components/FAB/index.js

marcaaron · 2021-04-14T00:28:46Z

@aliabbasmalik8 please do not force push.

marcaaron

LGTM

src/components/FAB/fabPropTypes.js

Luke9389 · 2021-04-14T01:19:04Z

Thanks @aliabbasmalik8!

OSBotify · 2021-04-19T22:41:56Z

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

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

aliabbasmalik8 requested a review from a team as a code owner April 8, 2021 19:40

MelvinBot requested review from Luke9389 and removed request for a team April 8, 2021 19:40

Luke9389 suggested changes Apr 9, 2021

View reviewed changes

aliabbasmalik8 force-pushed the IS-2151-fab-hidden-issue branch from bfa9c30 to 7ce5f8f Compare April 9, 2021 20:16

marcaaron requested changes Apr 13, 2021

View reviewed changes

marcaaron assigned aliabbasmalik8 Apr 13, 2021

[IS-2151] Fixed FAB position issue

9b673df

aliabbasmalik8 force-pushed the IS-2151-fab-hidden-issue branch from 7ce5f8f to 9b673df Compare April 13, 2021 23:29

marcaaron requested changes Apr 14, 2021

View reviewed changes

[IS-2151] make FAB main component simple

1bd334b

marcaaron previously approved these changes Apr 14, 2021

View reviewed changes

Luke9389 reviewed Apr 14, 2021

View reviewed changes

src/components/FAB/fabPropTypes.js Outdated Show resolved Hide resolved

[IS-2151] make variable name camelCase

c32c493

aliabbasmalik8 dismissed marcaaron’s stale review via c32c493 April 14, 2021 01:15

Luke9389 approved these changes Apr 14, 2021

View reviewed changes

Luke9389 merged commit e4e0273 into Expensify:master Apr 14, 2021

OSBotify mentioned this pull request Apr 14, 2021

Deploy Checklist: Expensify.cash 2021-04-13 #2380

Closed

66 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IS-2151] fixed FAB issue #2305

[IS-2151] fixed FAB issue #2305

aliabbasmalik8 commented Apr 8, 2021 •

edited

Loading

Luke9389 commented Apr 8, 2021

aliabbasmalik8 commented Apr 8, 2021 •

edited

Loading

Luke9389 commented Apr 8, 2021

Luke9389 commented Apr 9, 2021

Luke9389 Apr 9, 2021

aliabbasmalik8 Apr 9, 2021

Luke9389 Apr 9, 2021

aliabbasmalik8 Apr 9, 2021

aliabbasmalik8 commented Apr 9, 2021

marcaaron Apr 13, 2021

aliabbasmalik8 Apr 13, 2021 •

edited

Loading

marcaaron Apr 14, 2021

marcaaron Apr 14, 2021

marcaaron Apr 14, 2021

marcaaron Apr 14, 2021

marcaaron commented Apr 14, 2021

marcaaron left a comment

Luke9389 commented Apr 14, 2021

OSBotify commented Apr 19, 2021

[IS-2151] fixed FAB issue #2305

[IS-2151] fixed FAB issue #2305

Conversation

aliabbasmalik8 commented Apr 8, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iPad

Android

Luke9389 commented Apr 8, 2021

aliabbasmalik8 commented Apr 8, 2021 • edited Loading

Luke9389 commented Apr 8, 2021

Luke9389 commented Apr 9, 2021

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

aliabbasmalik8 commented Apr 9, 2021

Choose a reason for hiding this comment

aliabbasmalik8 Apr 13, 2021 • 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

marcaaron commented Apr 14, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Luke9389 commented Apr 14, 2021

OSBotify commented Apr 19, 2021

aliabbasmalik8 commented Apr 8, 2021 •

edited

Loading

aliabbasmalik8 commented Apr 8, 2021 •

edited

Loading

aliabbasmalik8 Apr 13, 2021 •

edited

Loading