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

Make referral banner dismissible #33925

Merged
merged 7 commits into from
Jan 24, 2024
Merged

Make referral banner dismissible #33925

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2866e5
make referral banner dismissable
gijoe0295 Jan 4, 2024
7f13d57
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe-32499
gijoe0295 Jan 12, 2024
15d87b1
dismiss money request banner
gijoe0295 Jan 12, 2024
21807d8
Merge branch 'main'
gijoe0295 Jan 16, 2024
f64a93a
reapply changes
gijoe0295 Jan 16, 2024
d9f6a59
increase pressable space
gijoe0295 Jan 16, 2024
bdc980d
fix lint
gijoe0295 Jan 16, 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
27 changes: 18 additions & 9 deletions src/components/OptionsSelector/BaseOptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import Button from '@components/Button';
import FixedFooter from '@components/FixedFooter';
import FormHelpMessage from '@components/FormHelpMessage';
import Icon from '@components/Icon';
import {Info} from '@components/Icon/Expensicons';
import {Close} from '@components/Icon/Expensicons';
import OptionsList from '@components/OptionsList';
import {PressableWithoutFeedback} from '@components/Pressable';
import ShowMoreButton from '@components/ShowMoreButton';
Expand Down Expand Up @@ -92,7 +92,7 @@ class BaseOptionsSelector extends Component {
allOptions,
focusedIndex,
shouldDisableRowSelection: false,
shouldShowReferralModal: false,
shouldShowReferralModal: this.props.shouldShowReferralCTA,
errorMessage: '',
paginationPage: 1,
value: '',
Expand Down Expand Up @@ -618,7 +618,7 @@ class BaseOptionsSelector extends Component {
</>
)}
</View>
{this.props.shouldShowReferralCTA && (
{this.props.shouldShowReferralCTA && this.state.shouldShowReferralModal && (
<View style={[this.props.themeStyles.ph5, this.props.themeStyles.pb5, this.props.themeStyles.flexShrink0]}>
<PressableWithoutFeedback
onPress={() => {
Expand Down Expand Up @@ -646,12 +646,21 @@ class BaseOptionsSelector extends Component {
{this.props.translate(`referralProgram.${this.props.referralContentType}.buttonText2`)}
</Text>
</Text>
<Icon
src={Info}
height={20}
width={20}
fill={this.props.theme.icon}
/>
<PressableWithoutFeedback
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the pressable and the icon have same size.

Screen.Recording.2024-01-11.at.3.31.42.PM.mov

Shouldn't we have some extra size for pressing? For now both the icon size and the pressing place size is 20

@dannymcclain yours thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dannymcclain what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call—ideally the pressable area of the x icon should be at least a 44px square.

Copy link
Contributor Author

@gijoe0295 gijoe0295 Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dannymcclain Can you confirm whether it is 40 or 44? 40 is the pressable area of the back (and other) button.

Screenshot 2024-01-13 at 02 52 30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if we do so, the button will move to the left like this:

Before Screenshot 2024-01-13 at 03 13 33
After Screenshot 2024-01-13 at 03 15 22

As the previous Info icon did not have pressable space as 40 either, I suggest we keep it the same, i.e. keep the pressable and the icon having the same size as 20.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha—40 would be acceptable if that's what we do elsewhere.

I see what you're talking about in your before and after. The right thing to do here IMO would be to adjust the padding on the actual banner so that the x icon gets the proper amount of clickable space without impacting the size of the banner/layout of the icon. @Expensify/design do y'all think it's worth it to adjust the banner to give that icon a proper amount of clickable area?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with that Danny. But I could have sworn we can also solve for this using React Native's hitslop, right? This we we can keep the button where it is but just define a greater hit area around around the button.

Copy link
Contributor Author

@gijoe0295 gijoe0295 Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@abdulrahuman5196 @dannymcclain I've updated, please check. I tried to keep everything visually the same though it requires some hard-coding for the padding.

Or we can do nothing and the hitSlop will be automatically calculated.

Before Screenshot 2024-01-16 at 14 37 41
After Screenshot 2024-01-16 at 14 34 19

onPress={this.handleReferralModal}
onMouseDown={(e) => {
e.preventDefault();
}}
role={CONST.ACCESSIBILITY_ROLE.BUTTON}
accessibilityLabel={this.props.translate('common.close')}
>
<Icon
src={Close}
height={20}
width={20}
fill={this.props.theme.icon}
/>
</PressableWithoutFeedback>
</PressableWithoutFeedback>
</View>
)}
Expand Down
Loading