Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Implemented Brave Rewards Ads feedback loop #465

Merged
merged 2 commits into from
Aug 7, 2019
Merged

Implemented Brave Rewards Ads feedback loop #465

merged 2 commits into from
Aug 7, 2019

Conversation

jasonrsadler
Copy link
Contributor

@jasonrsadler jasonrsadler commented May 13, 2019 •
edited
Loading

Partial for brave/brave-browser#4047

Changes

Implemented Ads feedback loop

Test plan

Link / storybook path to visual changes

Integration

  • Does this contain changes to src/components or src/

    • Will you publish to npm immediately after this PR, or wait until sometime in the future?
    • Incompatible API change to something existing (major version increase)
    • Adding new backwards-compatible functionality? (minor version increase)
    • Fixing a bug backwards-compatibly? (patch version increase)

  • Does this contain changes to src/features for brave-core?

    • Are there non backwards-compatible changes required for brave-core? Do not merge until brave-core PR is approvable. Link to brave-core PR:
    • Will you create brave-core PR to update to this commit after it is merged?
    • Wants uplift to brave-core feature branch?
      • When uplift-approved, merge to brave-core-0.VV.x feature branch
      • Create additional brave-core PRs for each feature branch to update commit

@jasonrsadler jasonrsadler self-assigned this May 13, 2019
@jasonrsadler jasonrsadler changed the title Implemented Brave Rewards Ads feedbacl loop Implemented Brave Rewards Ads feedback loop May 14, 2019
@jasonrsadler jasonrsadler force-pushed the ads-history branch 9 times, most recently from e780d16 to e2432bf Compare May 15, 2019 19:04
@jasonrsadler jasonrsadler marked this pull request as ready for review May 15, 2019 19:09
ryanml
ryanml reviewed May 15, 2019
View reviewed changes
package/package.json Outdated
@@ -2,7 +2,7 @@
"name": "brave-ui",
"main": "index.js",
"sideEffects": false,
"version": "0.38.0",
"version": "0.34.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to make updates to this file anymore (unless we are doing an NPM publish)

Was this inadvertent or did react-storybook-addon-chapters need that bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inadvertent for both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

ryanml
ryanml reviewed May 15, 2019
View reviewed changes
src/features/rewards/tableAdsHistory/index.tsx Outdated
this.getAdContent(row.adContent, i)
),
customStyle: {
width: '70% !important',
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we have these inline styles in the tableContribute component, but could we offload these to styled components? Let me know if there was some other limitation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offloaded. In fact, only the td was effective.

@jasonrsadler jasonrsadler force-pushed the ads-history branch 2 times, most recently from 9c8ee28 to 82218a7 Compare May 15, 2019 19:29
@jasonrsadler jasonrsadler requested a review from ryanml May 15, 2019 19:30
@jasonrsadler jasonrsadler force-pushed the ads-history branch 7 times, most recently from 2ceaee4 to f45234d Compare May 16, 2019 16:30
@jasonrsadler
Copy link
Contributor Author

Done.
Screen Shot 2019-07-12 at 2 05 10 PM

@jasonrsadler
Copy link
Contributor Author

cc @jenn-rhim
Filters updated

@jasonrsadler jasonrsadler removed their assignment Jul 29, 2019
@emerick emerick force-pushed the ads-history branch 2 times, most recently from 60fd667 to c702096 Compare August 1, 2019 17:43
NejcZdovc
NejcZdovc reviewed Aug 5, 2019
View reviewed changes
src/features/rewards/modalShowAdsHistory/index.tsx Outdated
id?: string
isMobile?: boolean
adsPerHour?: number
adsPerDay?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

let's improve naming on this one. This one is used to display Ads you've received in the past 7 days. Maybe totalDays?

Copy link
Collaborator

@tmancey tmancey Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

adsPerDay is correct which matches bat-native-ads and is the amount of ads per day. Unless @emerick this is not the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

this one is not the same

Copy link
Contributor

Choose a reason for hiding this comment

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

image

this variable is for this title

NejcZdovc
NejcZdovc reviewed Aug 5, 2019
View reviewed changes
src/features/rewards/modalShowAdsHistory/index.tsx Outdated
{getLocale('adsHistoryTitle')}
</StyledAdsHistoryTitle>
<StyledSubTitleText>
{getLocale('adsHistorySubTitle')}
Copy link
Contributor

Choose a reason for hiding this comment

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

this one needs to accept variable as currently is fixed to 7 Days. We need to pass in totalDays

NejcZdovc
NejcZdovc reviewed Aug 5, 2019
View reviewed changes
src/features/rewards/modalShowAdsHistory/index.tsx Outdated
{getLocale('adsCurrentlyViewing')}
</StyledAdsInfoText>
<StyledAdsPerHourText>
{(adsPerHour || '0') + (adsPerHour !== 1 ? getLocale('adsPerHourPlural') : getLocale('adsPerHourSingular'))}
Copy link
Contributor

Choose a reason for hiding this comment

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

this will not work for all languages. In Slovenian we have special variation for double as well

NejcZdovc
NejcZdovc reviewed Aug 5, 2019
View reviewed changes
src/features/rewards/tableAdsHistory/index.tsx Outdated
brandUrl: string
brandDisplayUrl: string
likeAction: number
adAction: 'click' | 'dismiss' | 'view' | 'landed'
Copy link
Contributor

Choose a reason for hiding this comment

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

let's export this as it could be useful for client when doing checks

@NejcZdovc
Copy link
Contributor

@emerick please check my previous comments about theme color and fonts. Thank you

@emerick emerick requested a review from NejcZdovc August 5, 2019 20:34
@emerick
Copy link
Collaborator

emerick commented Aug 5, 2019

@NejcZdovc Think I addressed all of the open feedback items, please take a look when you have a moment. Thanks!

@emerick emerick force-pushed the ads-history branch 6 times, most recently from 52fa96a to f81c208 Compare August 7, 2019 02:07
@emerick emerick merged commit 7f5c8ad into master Aug 7, 2019
@emerick emerick deleted the ads-history branch August 7, 2019 13:20
ryanml pushed a commit that referenced this pull request Sep 18, 2019
@emerick @ryanml
Merge pull request #465 from brave/ads-history
8382e79 
Implemented Brave Rewards Ads feedback loop
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@emerick emerick emerick left review comments

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@cezaraugusto cezaraugusto Awaiting requested review from cezaraugusto

@tmancey tmancey Awaiting requested review from tmancey

@ryanml ryanml Awaiting requested review from ryanml

Assignees
No one assigned
Labels
PR/pending-review rewards
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasonrsadler @emerick @NejcZdovc @jenn-rhim @tmancey @ryanml