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: missing copy icons and social icons on Android #20528

Merged
merged 11 commits into from
Jun 14, 2023
Merged

fix: missing copy icons and social icons on Android #20528

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
917e7b6
fix: copy icon in contact-methods & workspace-reimbursements page on …
sangar-1028 Jun 9, 2023
ed87945
fix: social icons refactor
sangar-1028 Jun 9, 2023
e713d2b
fix: conflicts
sangar-1028 Jun 9, 2023
51bdf0e
fix: comments to workaround
sangar-1028 Jun 12, 2023
26db64b
fix: WorkspaceBillsFirstSectioin
sangar-1028 Jun 13, 2023
511db7e
fix: mistake
sangar-1028 Jun 13, 2023
6706964
fix: reduce margin of social icons(parent)
sangar-1028 Jun 13, 2023
b2d1fcb
fix: remove todo comments
sangar-1028 Jun 14, 2023
d08df01
Merge branch 'main' into fix/issue-17368
sangar-1028 Jun 14, 2023
dec8ce3
fix: conflicts
sangar-1028 Jun 14, 2023
38ff87c
fix: lint error
sangar-1028 Jun 14, 2023
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
3 changes: 2 additions & 1 deletion src/components/PressableWithDelayToggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,10 @@ function PressableWithDelayToggle(props) {
<>
<Text
suppressHighlighting
style={[styles.mr1, ...props.textStyles]}
style={props.textStyles}
>
{props.isDelayButtonStateComplete && props.textChecked ? props.textChecked : props.text}
&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you replace mr1 with space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mr1 in this context doesn't work. That's why I replaced it with space

Copy link
Contributor

Choose a reason for hiding this comment

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

ok this regression came from another PR. But that's good to fix it here as it's simple.
Also I found another 2 regressions happening on production:

  • copy icon is not center aligned vertically
  • copy icon is not highlighted even though cursor changed to pointer changes to pointer
Screen.Recording.2023-06-13.at.11.48.27.AM.mov

This can be out of scope as it doesn't seem simple solution.
cc: @NikkiWines

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I think it's fine to make those separate issues but please note it in the PR so that our QA team doesn't mark them as regressions from this PR. Once this is live, they should be reported as new bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated

</Text>
<Pressable
ref={props.innerRef}
Expand Down
4 changes: 3 additions & 1 deletion src/pages/settings/Profile/Contacts/ContactMethodsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ const ContactMethodsPage = (props) => {
/>
<ScrollView>
<View style={[styles.ph5, styles.mv3, styles.flexRow, styles.flexWrap]}>
<Text>
{/* Workaround to fix https://github.com/Expensify/App/issues/17368.
TODO: Remove `numberOfLines` once https://github.com/facebook/react-native/pull/35703 is merged and applied to our repo */}
<Text numberOfLines={100}>
{props.translate('contacts.helpTextBeforeEmail')}
<CopyTextToClipboard
text="receipts@expensify.com"
Expand Down
2 changes: 1 addition & 1 deletion src/pages/signin/SignInPageLayout/Footer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ const Footer = (props) => {
</Hoverable>
))}
{i === 2 && (
<View style={styles.mt5}>
<View style={styles.mt4}>
<Socials />
</View>
)}
Expand Down
44 changes: 23 additions & 21 deletions src/pages/signin/Socials.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import React from 'react';
import {View} from 'react-native';
import _ from 'underscore';
import * as Link from '../../libs/actions/Link';
import Icon from '../../components/Icon';
import Text from '../../components/Text';
import PressableWithoutFeedback from '../../components/Pressable/PressableWithoutFeedback';
import * as Expensicons from '../../components/Icon/Expensicons';
import themeColors from '../../styles/themes/default';
import styles from '../../styles/styles';
import variables from '../../styles/variables';
import CONST from '../../CONST';
import Hoverable from '../../components/Hoverable';
import TextLink from '../../components/TextLink';

const socialsList = [
{
Expand All @@ -35,27 +34,30 @@ const socialsList = [
];

const Socials = () => (
<Text>
<View style={[styles.flexRow, styles.flexWrap]}>
{_.map(socialsList, (social) => (
<Hoverable key={social.link}>
{(hovered) => (
<View>
<TextLink
style={styles.pr1}
href={social.link}
>
<Icon
src={social.iconURL}
height={variables.iconSizeLarge}
width={variables.iconSizeLarge}
fill={hovered ? themeColors.link : themeColors.textLight}
/>
</TextLink>
</View>
<PressableWithoutFeedback
key={social.link}
href={social.link}
onPress={(e) => {
e.preventDefault();
Link.openExternalLink(social.link);
}}
accessible={false}
style={[styles.mr1, styles.mt1]}
shouldUseAutoHitSlop={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this change for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mr1 is for spacing between icons. mt1 is for row spacing in cases icons are wrapped.
If shouldUseAutoHitSlop is enabled, touch area of the pressable gets larger. To prevent this, we need to disable it

Copy link
Contributor

@aimane-chnaif aimane-chnaif Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see any difference between with shouldUseAutoHitSlop={false} and without.
Can you explain more detail with screenshots?

Edit: nvm, I found issue on iOS. I see now why hitSlop should not be applied here

>
{({hovered, pressed}) => (
<Icon
src={social.iconURL}
height={variables.iconSizeLarge}
width={variables.iconSizeLarge}
fill={hovered || pressed ? themeColors.link : themeColors.textLight}
/>
)}
</Hoverable>
</PressableWithoutFeedback>
))}
</Text>
</View>
);

Socials.displayName = 'Socials';
Expand Down
4 changes: 3 additions & 1 deletion src/pages/workspace/bills/WorkspaceBillsFirstSection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ const WorkspaceBillsFirstSection = (props) => {
containerStyles={[styles.cardSection]}
>
<View style={[styles.mv3]}>
<Text>
{/* Workaround to fix https://github.com/Expensify/App/issues/17368.
TODO: Remove `numberOfLines` once https://github.com/facebook/react-native/pull/35703 is merged and applied to our repo */}
<Text numberOfLines={100}>
{props.translate('workspace.bills.askYourVendorsBeforeEmail')}
{props.user.isFromPublicDomain ? (
<TextLink onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/7500/how-to-pay-your-company-bills-in-expensify/')}>
Expand Down
4 changes: 3 additions & 1 deletion src/pages/workspace/reimburse/WorkspaceReimburseView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,9 @@ class WorkspaceReimburseView extends React.Component {
]}
>
<View style={[styles.mv3, styles.flexRow, styles.flexWrap]}>
<Text>
{/* Workaround to fix https://github.com/Expensify/App/issues/17368.
TODO: Remove `numberOfLines` once https://github.com/facebook/react-native/pull/35703 is merged and applied to our repo */}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove these comments, they clutter up the code and we don't know when that PR is going to be merged.

Instead, I think it's better to link to facebook/react-native#35703 in the PR Details section and provide the same context there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated

Copy link
Contributor

Choose a reason for hiding this comment

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

@s-alves10 thanks for updating the code. What I meant above is that you should add details from that comment to the PR Details section (see screenshot) so that anyone who comes back to this PR in the future knows the context of why this was modified like this and what PR we're waiting on.

image

<Text numberOfLines={100}>
{this.props.translate('workspace.reimburse.captureNoVBACopyBeforeEmail')}
<CopyTextToClipboard
text="receipts@expensify.com"
Expand Down