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

Add provider to tip compliment #2555

Closed
wants to merge 1 commit into from
Closed

Add provider to tip compliment #2555

wants to merge 1 commit into from

Conversation

jdkuki
Copy link
Contributor

@jdkuki jdkuki commented May 30, 2019

Fixes brave/brave-browser#4586

Added publisher provider to tweetTip compliment string displayed when sharing a tip via tweet.
Compliment string now reads "I just tipped [publisher] on [provider]"

Submitter Checklist:

Test Plan:

See steps at brave/brave-browser#4586

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@jdkuki jdkuki requested a review from NejcZdovc May 30, 2019 23:41
@jdkuki jdkuki self-assigned this May 30, 2019
Remove deprecated API and replace with args->GetList()
Fixes brave/brave-browser#4586
return;
std::string tweet_id = args->GetList()[1].GetString();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tweet_id should be allowed to be empty so removed equivalent check used in old API

Copy link
Contributor

Choose a reason for hiding this comment

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

why should be allowed to be empty?

Copy link
Contributor Author

@jdkuki jdkuki Jun 1, 2019

Choose a reason for hiding this comment

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

It appears that tweet_id is given as empty in:

class TipSite extends React.Component<Props, {}> {
get actions () {
return this.props.actions
}
onTweet = () => {
let name = this.props.publisher.name
if (this.props.publisher.provider === 'twitter') {
const url = this.props.url
if (url && url.length > 0) {
name = `@${url.replace(/^.*\/(.*)$/, '$1')}`
}
}
this.actions.onTweet(name, '')
this.actions.onCloseDialog()
}

I believe the field may be used in the case of a twitter tip to attach the complement as a reply (may be completely wrong) and is not applicable in the case where we tip a site?

Copy link
Contributor Author

@jdkuki jdkuki Jun 1, 2019

Choose a reason for hiding this comment

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

tipSite.tsx:35 updated below

@@ -260,7 +260,7 @@
<message name="IDS_BRAVE_REWARDS_LOCAL_GRANT_ALREADY_CLAIMED_TEXT" desc="">Sorry, it appears that this grant has already been claimed.</message>
<message name="IDS_BRAVE_REWARDS_LOCAL_DONAT_NEXT_DATE" desc="Description of the date in the donation box">Next monthly tip date</message>
<message name="IDS_BRAVE_REWARDS_LOCAL_COMPLIMENT_TWEET" desc="">
I just tipped <ph name="NAME">$1<ex>user</ex></ph> using the Brave Browser. Check it out at https://brave.com/tips.
I just tipped <ph name="NAME">$1<ex>user</ex></ph> on <ph name="PROVIDER">$2<ex>site</ex></ph> using the Brave Browser. Check it out at https://brave.com/tips.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requires proper formatting, @NejcZdovc will suggest fix

@jdkuki jdkuki requested a review from ryanml June 4, 2019 16:24
@NejcZdovc
Copy link
Contributor

closing as stale, let's revisit when needed

@NejcZdovc NejcZdovc closed this Nov 9, 2019
@NejcZdovc NejcZdovc removed the request for review from ryanml November 9, 2019 08:39
@NejcZdovc NejcZdovc added this to the Closed milestone Mar 6, 2020
@bsclifton bsclifton deleted the provider-on-tweetTip branch October 21, 2020 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tweeting about a tip doesn't include youtube or twitch in canned tweet
2 participants