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

[$500] Right clicking email address shows pop up that says Copy URL to clipboard #8006

Closed
mvtglobally opened this issue Mar 4, 2022 · 28 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Mar 4, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Right click an email address in NewDot

Expected Result:

Pop up to say Copy email to clipboard

Actual Result:

Pop up says Copy URL to clipboard.
When you copy and paste, it shows mailto:testemail@gmail.com. We should only copy the email address and not include mailto:

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
image - 2022-03-04T001730 206

Upwork job link: https://www.upwork.com/jobs/~01f207936302328d06
Issue reported by: @mallenexpensify
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1646102764234269

View all open jobs on GitHub

@MelvinBot
Copy link

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

@mateusbra
Copy link
Contributor

mateusbra commented Mar 4, 2022

PROPOSAL

(if external)

  1. First we have to add translations to en.js and es.js:
//en.js
copyEmailToClipboard: 'Copy e-mail to clipboard',
//es.js
copyEmailToClipboard: 'Copiar e-mail al portapapeles',
  1. We should create a new action within ContextMenuActions.js:
    {
    textTranslateKey: 'reportActionContextMenu.copyURLToClipboard',
    icon: Expensicons.Clipboard,
    successTextTranslateKey: 'reportActionContextMenu.copied',
    successIcon: Expensicons.Checkmark,
    shouldShow: type => type === CONTEXT_MENU_TYPES.LINK,
    onPress: (closePopover, {selection}) => {
    Clipboard.setString(selection);
    hideContextMenu(true, ReportActionComposeFocusManager.focus);
    },

    add to line 39 removing mailto::
    {
        textTranslateKey:'reportActionContextMenu.copyEmailToClipboard',
        icon: Expensicons.Clipboard,
        successTextTranslateKey: 'reportActionContextMenu.copied',
        successIcon: Expensicons.Checkmark,
        shouldShow: type => type === CONTEXT_MENU_TYPES.EMAIL,
        onPress: (closePopover, {selection}) => {
            Clipboard.setString(selection.replace("mailto:",""));
            hideContextMenu(true, ReportActionComposeFocusManager.focus);
        },
    },

and create e-mail type here:

const CONTEXT_MENU_TYPES = {
LINK: 'LINK',
REPORT_ACTION: 'REPORT_ACTION',
};

add:
EMAIL: 'EMAIL',
3. As we only have the fileName prop with the URL/email value, we should create another prop doing the same as fileName but with a reliabler name:
const isAttachment = Boolean(htmlAttribs['data-expensify-source']);
const fileName = lodashGet(props.tnode, 'domNode.children[0].data', '');
const parentStyle = lodashGet(props.tnode, 'parent.styles.nativeTextRet', {});

Here we will add:

const link = lodashGet(props.tnode, 'domNode.children[0].data', '');

and also add the link prop within our anchorForCommentsOnlyPropTypes.js

  1. Now we have to call the properly context menu for the URL or Email here:
    ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

    Change this line to:
Str.isValidEmail(this.props.link) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

Note: We will have to do item 4. within index.js and index.native.js to cover native and non-native applications.

SCREEN RECORDING

2022-03-04.17-58-58.mp4

@madmax330 madmax330 added the External Added to denote the issue can be worked on by a contributor label Mar 7, 2022
@MelvinBot
Copy link

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

@michaelhaxhiu
Copy link
Contributor

Job is posted to upwork here.

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 10, 2022
@MelvinBot
Copy link

Triggered auto assignment to @marcochavezf (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@rahmanasad212
Copy link

well we can do this work by using this approach:

first of all i will add an onClick() function to the container that contains email.
secondly i will use Boolean state to open a dialog box that includes copy email to the clipbard and an icon to copy the mail.
lastly i will save that email in a state and can use where the user pastes it and after he/she has pasted it i can provide two options.
1- we can save the email until the next email comes in or we can erase it as soon as user has used it once.

thank you!

@rohailparacha
Copy link

I can create a button instead of ahref links. and Then I will use states to manage if it has been copied or not yet. and I will make sure following 3 steps are followed:

  1. Prevent the default behavior of the button (refresh the page);
  2. Run the copyToClipboard function;
  3. Add and removes some CSS classes, used for styling and notifying the user about the copy event

Secondly, for the mobile version, I will need to add a href again which will be hidden by default on desktop and toggled using media queries in the CSS:

@mollfpr
Copy link
Contributor

mollfpr commented Mar 10, 2022

Proposal

  1. Update ContextMenuActions when handle press event to replace mailto: from email selection
    onPress: (closePopover, {selection}) => {
    Clipboard.setString(selection);
    hideContextMenu(true, ReportActionComposeFocusManager.focus);
    },
        onPress: (closePopover, {selection}) => {
            Clipboard.setString(selection.replace('mailto:', ''));
            hideContextMenu(true, ReportActionComposeFocusManager.focus);
        },
  1. I create a function to handle whether need to return the label with Copy URL to clipboard or Copy email to clipboard in BaseReportActionContextMenu
    text={this.props.translate(contextAction.textTranslateKey)}
    translatedText(key) {
        // Check wether selection is email or not
        if (this.props.type === CONTEXT_MENU_TYPES.LINK && this.props.selection.includes('mailto:')) {
            return this.props.translate('reportActionContextMenu.copyEmailToClipboard');
        }

        return this.props.translate(key);
    }
                        text={this.translatedText(contextAction.textTranslateKey)}
Screen.Recording.2022-03-10.at.22.18.27.mov

@Santhosh-Sellavel
Copy link
Collaborator

Reviewing Proposals now

@Santhosh-Sellavel
Copy link
Collaborator

@mateusbra
Can you update your proposal, without using this.props.fileName that does not make any sense. Also after copying the email, on pasting the value. Ex: It should be adf@mail.com not mailto:adf@mail.com.

@mateusbra
Copy link
Contributor

mateusbra commented Mar 14, 2022

Proposal updated!

@michaelhaxhiu michaelhaxhiu changed the title Right clicking email address shows pop up that says Copy URL to clipboard [$500] Right clicking email address shows pop up that says Copy URL to clipboard Mar 16, 2022
@michaelhaxhiu
Copy link
Contributor

Doubling price to $500

@Santhosh-Sellavel
Copy link
Collaborator

@mateusbra

Note: We will have to do item 3. within index.js and index.native.js to cover native and non-native applications.

It's item 4, not item 3. Because you added a new step 3. This is why I recommend writing an updated proposal in a new comment.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Mar 16, 2022

@marcochavezf
🎀👀🎀 C+ reviewed

So far @mateusbra proposal looks good to me!

But here,

  1. As we only have the fileName prop with the URL/email value, we should create another prop doing the same as fileName but with a reliabler name:
    App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 19 to 21 in 6e0bffe

const isAttachment = Boolean(htmlAttribs['data-expensify-source']);
const fileName = lodashGet(props.tnode, 'domNode.children[0].data', '');
const parentStyle = lodashGet(props.tnode, 'parent.styles.nativeTextRet', {});
Here we will add:

const link = lodashGet(props.tnode, 'domNode.children[0].data', '');

I suggest using generic prop name instead of fileName like anchorText or displayName or displayLabel. To avoid repeating the same value in two props. Please post your thoughts on this.

@mateusbra
Copy link
Contributor

I suggest using generic prop name instead of fileName like anchorText or displayName or displayLabel. To avoid repeating the same value in two props. Please post your thoughts on this.

I agree with that, using a generic prop name is better than having two different props that does the same.

@marcochavezf
Copy link
Contributor

Thanks @Santhosh-Sellavel! Assigning @mateusbra

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 16, 2022
@MelvinBot
Copy link

📣 @mateusbra You have been assigned to this job by @marcochavezf!
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 📖

@mateusbra
Copy link
Contributor

Applied on Upwork, once I'm hired I'll rise a PR.

@mallenexpensify
Copy link
Contributor

Hired @mateusbra , @Santhosh-Sellavel can you apply too please for Contributor+? https://www.upwork.com/jobs/~01f207936302328d06

@mateusbra mateusbra mentioned this issue Mar 17, 2022
84 tasks
@Santhosh-Sellavel
Copy link
Collaborator

Done! @mallenexpensify

@michaelhaxhiu
Copy link
Contributor

Hired 👍

@michaelhaxhiu
Copy link
Contributor

Update on behalf of @mateusbra -- PR was merged to staging 4 days ago. Looks like we discovered another bug that @mateusbra reported #8311.

@MelvinBot MelvinBot removed the Overdue label Mar 28, 2022
@michaelhaxhiu
Copy link
Contributor

Can you provide an update on this one please?

@mateusbra
Copy link
Contributor

Hey @michaelhaxhiu, I think this was deployed to production has 6 days from #8195 (comment)

@michaelhaxhiu
Copy link
Contributor

I noticed iOS failed to deploy and was curious if that changes anything

@Santhosh-Sellavel
Copy link
Collaborator

@michaelhaxhiu I can see the issue fix is working fine in the current production version without any regression. We are good to go here, if needed please check this internally and update us on payment status here thanks!

@michaelhaxhiu
Copy link
Contributor

@Santhosh-Sellavel thanks for the input, makes sense - it was a false alarm on my side!

Both paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests