Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix]: Incorrect padding in group invite flow in RHP #42728

Merged
merged 5 commits into from
Jun 5, 2024
Merged

[fix]: Incorrect padding in group invite flow in RHP #42728

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa5f5d4
Fix styles
allgandalf May 28, 2024
cbbeeeb
Merge branch 'Expensify:main' into issue42594
allgandalf May 30, 2024
62d4d85
Merge branch 'Expensify:main' into issue42594
allgandalf May 30, 2024
47d2391
make footer component
allgandalf May 31, 2024
dc88f9d
remove view
allgandalf May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 31 additions & 27 deletions src/pages/InviteReportParticipantsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React, {useCallback, useEffect, useMemo, useState} from 'react';
import type {SectionListData} from 'react-native';
import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import FormAlertWithSubmitButton from '@components/FormAlertWithSubmitButton';
Expand Down Expand Up @@ -187,10 +186,25 @@ function InviteReportParticipantsPage({betas, personalDetails, report, didScreen
return OptionsListUtils.getHeaderMessage(invitePersonalDetails.length !== 0, Boolean(userToInvite), searchValue);
}, [searchTerm, userToInvite, excludedUsers, invitePersonalDetails, translate, reportName]);

const footerContent = useMemo(
() => (
<FormAlertWithSubmitButton
isDisabled={!selectedOptions.length}
buttonText={translate('common.invite')}
onSubmit={inviteUsers}
containerStyles={[styles.flexReset, styles.flexGrow0, styles.flexShrink0, styles.flexBasisAuto]}
enabledWhenOffline
disablePressOnEnter
/>
),
[selectedOptions.length, inviteUsers, translate, styles],
);

return (
<ScreenWrapper
shouldEnableMaxHeight
testID={InviteReportParticipantsPage.displayName}
includeSafeAreaPaddingBottom={false}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, what does this do and why do we want it to be false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inline with our Guidelines:

App/contributingGuides/FORMS.md

Lines 321 to 326 in 79406ff

### Safe Area Padding
Any `FormProvider.js` that has a button will also add safe area padding by default. If the `<FormProvider>` is inside a `<ScreenWrapper>`, we will want to disable the default safe area padding applied there e.g.
```jsx
<ScreenWrapper includeSafeAreaPaddingBottom={false}>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addtional context:

App/src/components/ScreenWrapper.tsx

Lines 236 to 239 in 79406ff

// We always need the safe area padding bottom if we're showing the offline indicator since it is bottom-docked.
if (includeSafeAreaPaddingBottom || (isOffline && shouldShowOfflineIndicator)) {
paddingStyle.paddingBottom = paddingBottom;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain it to me like I am a child? 😄 Just to make sure I understand what it does and why we are setting it to false. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeahhh, so we display offline indicator when the user isn't connected to internet, we want to add extra bottom padding whenever we want to display the offline indicator, why you may ask, well we have a bottom bar for iOS, if we do not add extra padding then the offline indicator will be inline with the bottom bar, rest of the times, we do not want the extra padding to be present.

App/src/components/ScreenWrapper.tsx

Lines 236 to 239 in 79406ff

// We always need the safe area padding bottom if we're showing the offline indicator since it is bottom-docked.
if (includeSafeAreaPaddingBottom || (isOffline && shouldShowOfflineIndicator)) {
paddingStyle.paddingBottom = paddingBottom;
}

But as you can see above extra bottom padding will be added if includeSafeAreaPaddingBottom is true even when the user is online, hence we explicitly set includeSafeAreaPaddingBottom to false and let the extra padding come when the user is offline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go,

If we set includeSafeAreaPaddingBottom to true:

Online Offline
Screenshot 2024-06-03 at 8 43 16 PM Screenshot 2024-06-03 at 8 42 14 PM

If we set includeSafeAreaPaddingBottom to false:

Online Offline
Screenshot 2024-06-03 at 8 44 18 PM Screenshot 2024-06-03 at 8 44 56 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, yeah I think that helps validate that we should always be using true for iOS/Android.

Copy link
Contributor Author

@allgandalf allgandalf Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool to conforming the requirements once:

  • For Desktop/Browser: We would set includeSafeAreaPaddingBottom to false.

  • For Android Native** and iOS Native** : We would set includeSafeAreaPaddingBottom to true.

Also, as mentioned we would need to make changes throughout the codebase where ever we have used this bottom button, so should we make a new issue for this or do all those changes in this PR itself @shawnborton ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine if we have a new issue for this one so we don't block progress on this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on that :)

>
<HeaderWithBackButton
title={translate('workspace.invite.members')}
Expand All @@ -199,32 +213,22 @@ function InviteReportParticipantsPage({betas, personalDetails, report, didScreen
Navigation.goBack(backRoute);
}}
/>
<View style={[styles.flex1, styles.p1]}>
<SelectionList
canSelectMultiple
sections={sections}
ListItem={InviteMemberListItem}
textInputLabel={translate('selectionList.nameEmailOrPhoneNumber')}
textInputValue={searchTerm}
onChangeText={setSearchTerm}
headerMessage={headerMessage}
onSelectRow={toggleOption}
onConfirm={inviteUsers}
showScrollIndicator
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
showLoadingPlaceholder={!didScreenTransitionEnd || !OptionsListUtils.isPersonalDetailsReady(personalDetails)}
/>
</View>
<View style={[styles.flexShrink0, styles.p5]}>
<FormAlertWithSubmitButton
isDisabled={!selectedOptions.length}
buttonText={translate('common.invite')}
onSubmit={inviteUsers}
containerStyles={[styles.flexReset, styles.flexGrow0, styles.flexShrink0, styles.flexBasisAuto, styles.mb5]}
enabledWhenOffline
disablePressOnEnter
/>
</View>

<SelectionList
canSelectMultiple
sections={sections}
ListItem={InviteMemberListItem}
textInputLabel={translate('selectionList.nameEmailOrPhoneNumber')}
textInputValue={searchTerm}
onChangeText={setSearchTerm}
headerMessage={headerMessage}
onSelectRow={toggleOption}
onConfirm={inviteUsers}
showScrollIndicator
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
showLoadingPlaceholder={!didScreenTransitionEnd || !OptionsListUtils.isPersonalDetailsReady(personalDetails)}
footerContent={footerContent}
/>
</ScreenWrapper>
);
}
Expand Down
Loading