Inline tipping media data is now generalized #2723

jasonrsadler · 2019-06-18T02:12:17Z

Fixes brave/brave-browser#4907

Submitter Checklist:

Test Plan:

Follow test plan at #2665 and #2195

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.

jasonrsadler · 2019-06-18T03:15:09Z

Blocked on #2665

emerick · 2019-06-22T15:27:40Z

common/extensions/api/brave_rewards.json

              "userId": {
                "type": "string",
                "description": "User ID of tweet author"
              },
-              "name": {
+              "twitterName": {
                "type": "string",
                "description": "Name of tweet author"


Does this description need an update as well?

I don't think so. This is still going to a twitter designated function.

I see, I misread that diff!

emerick

LGTM

NejcZdovc · 2019-06-22T16:20:19Z

@jasonrsadler is it possible to merge BraveRewardsTipTwitterUserFunction and BraveRewardsTipRedditUserFunction now that we have the same structure?

NejcZdovc · 2019-06-22T16:22:24Z

components/brave_rewards/resources/extension/brave_rewards/background.ts

@@ -79,7 +79,8 @@ chrome.runtime.onConnect.addListener(function () {
  })
 })

-const tipTwitterUser = (tweetMetaData: RewardsTip.TweetMetaData) => {
+const tipTwitterMedia = (mediaMetaData: RewardsTip.MediaMetaData) => {


can we merge tipTwitterMedia and tipTwitterMedia?

see comment

components/brave_rewards/resources/tip/components/app.tsx

NejcZdovc · 2019-06-22T16:24:27Z

components/brave_rewards/resources/tip/components/app.tsx

-  getTipBanner = (url: string, publisher: RewardsTip.Publisher, tweetMetaData?: RewardsTip.TweetMetaData, redditMetaData?: RewardsTip.RedditMetaData) => {
-    if (tweetMetaData) {
+  getTipBanner = (url: string, publisher: RewardsTip.Publisher, mediaMetaData: RewardsTip.MediaMetaData | undefined) => {
+    if (mediaMetaData && mediaMetaData.mediaType === 'twitter') {
      return (
        <TipTwitterUser


Do we need separate components for TipTwitterUserand TipRedditUser?

NejcZdovc · 2019-06-22T16:25:36Z

components/brave_rewards/resources/tip/components/siteBanner.tsx

+      return
+    }
+
+    if (!this.props.mediaMetaData ||


this check is not needed as you check at the top already

NejcZdovc · 2019-06-22T16:26:23Z

components/brave_rewards/resources/tip/components/siteBanner.tsx

-    if (!this.props.tweetMetaData ||
-        !this.props.tweetMetaData.tweetText ||
-        this.props.tweetMetaData.tweetText.length === 0) {
+    if (!this.props.mediaMetaData || this.props.mediaMetaData.mediaType !== 'twitter') {


could we save this.props.mediaMetaData into mediaMetaData so that it's cleaner

NejcZdovc · 2019-06-22T16:26:36Z

components/brave_rewards/resources/tip/components/siteBanner.tsx

-    if (!this.props.redditMetaData ||
-      !this.props.redditMetaData.postText ||
-      this.props.redditMetaData.postText.length === 0) {
+    if (!this.props.mediaMetaData || this.props.mediaMetaData.mediaType !== 'reddit') {


same comments as for twitter text

NejcZdovc · 2019-06-22T16:27:22Z

components/brave_rewards/resources/tip/components/siteBanner.tsx

@@ -210,9 +216,9 @@ class Banner extends React.Component<Props, State> {
        addFundsLink={this.addFundsLink}
      >
      {
-        this.props.tweetMetaData
+        this.props.mediaMetaData && this.props.mediaMetaData.mediaType === 'twitter'


let's check first for !this.props.mediaMetaData so that we don't need to check for every platform

NejcZdovc · 2019-06-22T16:27:55Z

components/brave_rewards/resources/tip/components/tipRedditUser.tsx

-      redditMetaData &&
-        redditMetaData.postText &&
-        redditMetaData.postText.length > 0
+      mediaMetaData &&


this should be on line 40

file is removed

jasonrsadler · 2019-06-22T19:35:13Z

@jasonrsadler is it possible to merge BraveRewardsTipTwitterUserFunction and BraveRewardsTipRedditUserFunction now that we have the same structure?

I attempted to on the first implementation but in the background.ts portion that gets msg.mediaMetaData, everything builds but at runtime the correct union type can't be inferred and causes missing param errors.

ryanml · 2019-06-24T21:17:43Z

components/brave_rewards/resources/tip/components/tipMediaUser.tsx

+
+    if (mediaMetaData.mediaType === 'twitter') {
+      const key =
+        mediaMetaData &&


Since this is inside a conditional for mediaMetaData can we remove this check?

ryanml · 2019-06-24T21:17:55Z

components/brave_rewards/resources/tip/components/tipMediaUser.tsx

+      publisher.title = getLocale(key, { user: mediaMetaData.screenName })
+    } else if (mediaMetaData.mediaType === 'reddit') {
+      const key =
+        mediaMetaData &&


same as above

emerick

LGTM

jasonrsadler changed the title ~~Media inline~~ Inline tipping media data is now generalized Jun 18, 2019

jasonrsadler self-assigned this Jun 18, 2019

jasonrsadler added feature/rewards Uplift/Blocked labels Jun 18, 2019

jasonrsadler force-pushed the media-inline branch from 55268e1 to 23beae3 Compare June 18, 2019 03:00

jasonrsadler added this to the 0.68.x - Nightly milestone Jun 18, 2019

jasonrsadler force-pushed the media-inline branch from 23beae3 to bb17c03 Compare June 18, 2019 03:13

jasonrsadler changed the base branch from master to reddit-inline June 18, 2019 11:17

jasonrsadler force-pushed the media-inline branch 4 times, most recently from cdb70b6 to a700b92 Compare June 18, 2019 12:07

jasonrsadler force-pushed the reddit-inline branch 17 times, most recently from eaa3a4f to 93fc6aa Compare June 21, 2019 02:00

jasonrsadler force-pushed the media-inline branch from 63a5593 to c2d86dd Compare June 22, 2019 02:19

emerick reviewed Jun 22, 2019

View reviewed changes

emerick self-requested a review June 22, 2019 15:52

emerick previously approved these changes Jun 22, 2019

View reviewed changes

NejcZdovc reviewed Jun 22, 2019

View reviewed changes

components/brave_rewards/resources/tip/components/app.tsx Outdated Show resolved Hide resolved

NejcZdovc reviewed Jun 22, 2019

View reviewed changes

jasonrsadler dismissed emerick’s stale review via 720bd26 June 22, 2019 23:37

jasonrsadler force-pushed the media-inline branch 2 times, most recently from 720bd26 to 8437a56 Compare June 23, 2019 02:30

jasonrsadler requested a review from NejcZdovc June 23, 2019 04:41

ryanml reviewed Jun 24, 2019

View reviewed changes

Fixes brave/brave-browser#4907

8ab359e

jasonrsadler force-pushed the media-inline branch from 8437a56 to 8ab359e Compare June 25, 2019 10:14

jasonrsadler requested a review from ryanml June 25, 2019 10:15

jasonrsadler modified the milestones: 0.68.x - Dev, 0.69.x - Nightly Jul 8, 2019

emerick approved these changes Jul 22, 2019

View reviewed changes

jasonrsadler merged commit e3d935a into master Jul 22, 2019

jasonrsadler deleted the media-inline branch July 22, 2019 17:30

petemill mentioned this pull request Jul 22, 2019

Support Tips for new Twitter site #2943

Merged

32 tasks

NejcZdovc removed the PR/Pending Review label Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline tipping media data is now generalized #2723

Inline tipping media data is now generalized #2723

jasonrsadler commented Jun 18, 2019 •

edited

Loading

jasonrsadler commented Jun 18, 2019

emerick Jun 22, 2019

jasonrsadler Jun 22, 2019

emerick Jun 22, 2019

emerick left a comment

NejcZdovc commented Jun 22, 2019

NejcZdovc Jun 22, 2019

jasonrsadler Jun 22, 2019

NejcZdovc Jun 22, 2019

jasonrsadler Jun 23, 2019

NejcZdovc Jun 22, 2019

jasonrsadler Jun 23, 2019

NejcZdovc Jun 22, 2019

jasonrsadler Jun 23, 2019

NejcZdovc Jun 22, 2019

jasonrsadler Jun 23, 2019

NejcZdovc Jun 22, 2019

jasonrsadler Jun 23, 2019

NejcZdovc Jun 22, 2019

jasonrsadler Jun 23, 2019

jasonrsadler commented Jun 22, 2019

ryanml Jun 24, 2019

jasonrsadler Jun 25, 2019

ryanml Jun 24, 2019

jasonrsadler Jun 25, 2019

emerick left a comment

Inline tipping media data is now generalized #2723

Inline tipping media data is now generalized #2723

Conversation

jasonrsadler commented Jun 18, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

jasonrsadler commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick left a comment

Choose a reason for hiding this comment

NejcZdovc commented Jun 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler commented Jun 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick left a comment

Choose a reason for hiding this comment

jasonrsadler commented Jun 18, 2019 •

edited

Loading