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.

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

[HOLD for payment 2022-05-20][$250] Android/IOS - Link is not aligned vertically in pay bills section on public domain @thesahindia #8664

Closed
kbecciv opened this issue Apr 17, 2022 · 33 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Monthly KSv2 Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Apr 17, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. launch the app
  2. Log in with gmail account
  3. Go to any Workspace - Bills

Expected Result:

Link is aligned vertically in pay bills section on public domain

Actual Result:

Link is not aligned vertically in pay bills section on public domain

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.1.55.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any public account

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Screenshot_20220416-231156_New Expensify

Expensify/Expensify Issue URL:

Issue reported by: @thesahindia

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1639149882009500

View all open jobs on GitHub

@kbecciv kbecciv added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Apr 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 17, 2022

Triggered auto assignment to @sakluger (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member labels Apr 17, 2022
@thesahindia
Copy link
Member

Proposal

<TouchableOpacity
onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/7500/how-to-pay-your-company-bills-in-expensify/')}
>

Using Text instead of TouchableOpacity will fix this.

    <Text
      style={[styles.link]}
      onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/7500/how-to-pay-your-company-bills-in-expensify/')}
    >
      [email protected]
    </Text>

Shouldn't we add a NewWindow icon here because it's a hyperlink?

@sakluger
Copy link
Contributor

This is working fine for me on iOS. I'm going to see if I can find someone with Android who can reproduce the issue.

@thesahindia
Copy link
Member

@sakluger, to repro this you need an account made with public domain email.

@strepanier03
Copy link
Contributor

@sakluger -- I was able to repro on Android:

  1. Open New Expensify app,
  2. Login with a public domain email (ex: I used [email protected])
  3. Go to Workspace
  4. Pay Bills

Screenshot_20220419-110845

@sakluger
Copy link
Contributor

Thanks @strepanier03 for verifying! I think this is limited to Android since I couldn't reproduce on iOS. I'll update the original description and pass along to engineering triage.

@sakluger sakluger added Engineering Improvement Item broken or needs improvement. labels Apr 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 19, 2022

Triggered auto assignment to @yuwenmemon (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@sakluger sakluger assigned yuwenmemon and unassigned yuwenmemon and sakluger Apr 19, 2022
@yuwenmemon yuwenmemon added the External Added to denote the issue can be worked on by a contributor label Apr 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 20, 2022

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@yuwenmemon yuwenmemon added Weekly KSv2 and removed Daily KSv2 labels Apr 20, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Apr 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 20, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

@parasharrajat

This comment was marked as outdated.

@thesahindia

This comment was marked as resolved.

@parasharrajat
Copy link
Member

Yes

@thesahindia

This comment was marked as resolved.

@parasharrajat

This comment was marked as outdated.

@thesahindia

This comment was marked as resolved.

@parasharrajat
Copy link
Member

Can you share the screenshot? I have seen a few places where this icon is used. Let's see what others have to say. cc: @stitesExpensify @NicMendonca

@thesahindia
Copy link
Member

thesahindia commented Apr 25, 2022

Yeah sure.

Screenshots

Simulator Screen Shot - iPhone 13 - 2022-04-25 at 22 33 16

Simulator Screen Shot - iPhone 13 - 2022-04-25 at 22 33 09

@Justicea83
Copy link
Contributor

Justicea83 commented Apr 26, 2022

Proposal

TouchableOpacity doesn't work well in android according to this issue.

A work around it is

  1. Refactor the codes in components/CopyTextToClipboard.js.

Update the PropTypes

const propTypes = {
    /** The text to display and copy to the clipboard */
    text: PropTypes.string.isRequired,

    /** Styles to apply to the text */
    textStyles: PropTypes.arrayOf(PropTypes.object),

    /** The default icon to show */
    icon: PropTypes.func,

    /* Whether to show the text or not */
    showText: PropTypes.bool,

    ...withLocalizePropTypes,
}; 

Update Default Prop Types

const defaultProps = {
    textStyles: [],
    icon: Expensicons.Clipboard,
    showText: true,
};

Update the render function to this

render() {
        return (
            <Text
                onPress={this.copyToClipboard}
                style={[styles.flexRow, styles.cursorPointer]}
            >
                {this.props.showText && <Text style={this.props.textStyles}>{this.props.text}</Text>}
                <Tooltip text={this.props.translate('reportActionContextMenu.copyToClipboard')}>
                    <Icon
                        src={this.state.showCheckmark ? Expensicons.Checkmark : this.props.icon}
                        fill={this.state.showCheckmark ? themeColors.iconSuccessFill : themeColors.icon}
                        width={variables.iconSizeSmall}
                        height={variables.iconSizeSmall}
                        inline
                    />
                </Tooltip>
            </Text>
        );
    }

2.Refactor the codes in workspace/bills/WorkspaceBillsFirstSection.js.

Replace https://github.com/Expensify/App/blob/main/src/pages/workspace/bills/WorkspaceBillsFirstSection.js#L2 with

import {View, TouchableOpacity, TouchableNativeFeedback, Platform} from 'react-native';

Replace the codes in https://github.com/Expensify/App/blob/main/src/pages/workspace/bills/WorkspaceBillsFirstSection.js#L56-L61 with

               {
                                Platform.OS === 'android'
                                    ? (
                                        <TouchableNativeFeedback
                                            onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/7500/how-to-pay-your-company-bills-in-expensify/')}
                                        >
                                            <Text style={[styles.textBlue]}>[email protected]</Text>
                                        </TouchableNativeFeedback>
                                    )
                                    : (
                                        <TouchableOpacity
                                            onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/7500/how-to-pay-your-company-bills-in-expensify/')}
                                        >
                                            <Text style={[styles.textBlue]}>[email protected]</Text>
                                        </TouchableOpacity>
                                    )
                            }

So were are rendering TouchableNativeFeedback if the platform is android otherwise the TouchableOpacity is rendered.

That should work fine, thats the output in android. Everything shows the same in the other platforms.

Screenshot 2022-04-26 at 12 19 14 PM

@parasharrajat
Copy link
Member

parasharrajat commented Apr 26, 2022

@parasharrajat
Copy link
Member

Based on slack, the final expected behavior is Link is aligned vertically in pay bills section on public domain.

@Justicea83
Copy link
Contributor

Assigning myself since my proposal was accepted

@Justicea83 Justicea83 self-assigned this Apr 27, 2022
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 27, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 27, 2022

📣 @Justicea83 You have been assigned to this job by @Justicea83!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@thesahindia
Copy link
Member

I think we can go with the approach that I had proposed earlier instead of using two different Touchable

Proposal

<TouchableOpacity
onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/7500/how-to-pay-your-company-bills-in-expensify/')}
>

Using Text instead of TouchableOpacity will fix this.

    <Text
      style={[styles.link]}
      onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/7500/how-to-pay-your-company-bills-in-expensify/')}
    >
      [email protected]
    </Text>

What you think @parasharrajat, @stitesExpensify ?

@parasharrajat
Copy link
Member

Sorry, @thesahindia but this is already being worked on.

@NicMendonca
Copy link
Contributor

NicMendonca commented Apr 28, 2022

@thesahindia @parasharrajat don't forget to apply for the job for reporting/ reviewing payment, thank you!

@thesahindia
Copy link
Member

Done ✅

@mallenexpensify mallenexpensify changed the title [$250] Android/IOS - Link is not aligned vertically in pay bills section on public domain @thesahindia [HOLD for payment 2022-05-20][$250] Android/IOS - Link is not aligned vertically in pay bills section on public domain @thesahindia May 20, 2022
@mallenexpensify mallenexpensify added the Awaiting Payment Auto-added when associated PR is deployed to production label May 20, 2022
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels May 23, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 23, 2022

This issue has not been updated in over 15 days. @parasharrajat, @NicMendonca, @Justicea83, @stitesExpensify eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@NicMendonca
Copy link
Contributor

@parasharrajat @thesahindia I am so sorry, this job post expired before I could issue payment 😞 Can you please accept my offer?

@thesahindia
Copy link
Member

No prob. Accepted the offer ✅

@NicMendonca
Copy link
Contributor

paid - sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants