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

Tip banner display is now customized for Reddit inline tipping #498

Merged
merged 1 commit into from
Jun 14, 2019
Merged

Tip banner display is now customized for Reddit inline tipping #498

merged 1 commit into from
Jun 14, 2019

Conversation

jasonrsadler
Copy link
Contributor

@jasonrsadler jasonrsadler commented Jun 12, 2019 •
edited
Loading

Changes

Test plan

Link / storybook path to visual changes

https://brave-ui-3ev1bujlm.now.sh/?path=/story/feature-components-rewards-concepts-desktop--site-banner

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 Jun 12, 2019
@jasonrsadler jasonrsadler force-pushed the reddit-inline branch 7 times, most recently from 7523209 to a89b07b Compare June 12, 2019 18:08
@jasonrsadler jasonrsadler changed the title Change onTweet to onMediaAction for use from other media providers Tip banner display is now customized for Reddit inline tipping Jun 12, 2019
@jasonrsadler jasonrsadler force-pushed the reddit-inline branch 2 times, most recently from df7f6db to 93107ec Compare June 12, 2019 18:59
NejcZdovc
NejcZdovc reviewed Jun 13, 2019
View reviewed changes
stories/features/rewards/concepts.tsx Outdated
@@ -100,6 +104,10 @@ storiesOf('Feature Components/Rewards/Concepts/Desktop', module)
return screenName.length > 0 || tweetText.length > 0
}

const isRedditTip = () => {
return !isTwitterTip() && (redditUserName.length !== 0 && redditPost.length !== 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this check !isTwitterTip() as isRedditTip() is else to twitter check

Copy link
Contributor Author

@jasonrsadler jasonrsadler Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No, it is not needed. removed

@jasonrsadler jasonrsadler force-pushed the reddit-inline branch 2 times, most recently from 8629dc2 to 9926ce7 Compare June 13, 2019 17:11
NejcZdovc
NejcZdovc reviewed Jun 13, 2019
View reviewed changes
src/features/rewards/mediaBox/index.tsx Outdated
<StyledMediaBox>
<StyledMediaIcon>
{
this.props.mediaType === 'twitter'
Copy link
Contributor

Choose a reason for hiding this comment

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

could you extract this into separate function, so that render is small

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

NejcZdovc
NejcZdovc reviewed Jun 13, 2019
View reviewed changes
src/features/rewards/mediaBox/index.tsx Outdated
}
</StyledMediaIcon>
<StyledMediaTimestamp>
{
Copy link
Contributor

Choose a reason for hiding this comment

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

could you extract this into separate function, so that render is small

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

NejcZdovc
NejcZdovc reviewed Jun 13, 2019
View reviewed changes
stories/features/rewards/concepts.tsx Outdated
@@ -83,6 +83,9 @@ storiesOf('Feature Components/Rewards/Concepts/Desktop', module)
.add('Site Banner', withState({ donationAmounts, currentAmount: '5.0', showBanner: true }, (store) => {
const screenName = text('Screen Name', '')
const tweetText = text('Tweet Text', '')
const redditUserName = text('Reddit User Name', '')
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 try to use same knobs and have select with types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

NejcZdovc
NejcZdovc reviewed Jun 13, 2019
View reviewed changes
stories/features/rewards/other.tsx Outdated
tweetTimestamp={number('Timestamp in seconds', 46420000)}
tweetText={text('Tweet text', 'This is my tweet.')}
<MediaBox
mediaType={'twitter'}
Copy link
Contributor

Choose a reason for hiding this comment

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

make this select option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ryanml
ryanml approved these changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

++ other feedback has been addressed, lgtm

NejcZdovc
NejcZdovc reviewed Jun 14, 2019
View reviewed changes
stories/features/rewards/concepts.tsx Outdated
@@ -113,7 +118,7 @@ storiesOf('Feature Components/Rewards/Concepts/Desktop', module)
<SiteBanner
domain={text('Domain', 'duckduckgo.com')}
name={text('Name', 'duckduckgo.com')}
screenName={screenName}
screenName={screenName || ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed? is it possible to be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Removed

NejcZdovc
NejcZdovc reviewed Jun 14, 2019
View reviewed changes
src/features/rewards/mediaBox/index.tsx
export interface Props {
mediaType: 'twitter' | 'reddit'
mediaText: string
mediaTimestamp: number
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need mediaTimestamp and mediaTimetext? Wouldn't time displayed in the box be 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.

not sure why twitter would have different format then reddit

Copy link
Contributor

Choose a reason for hiding this comment

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

we should try to have box as general as possible so when we add github we would just add timestamp in and everything would just work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed via DM

@jasonrsadler jasonrsadler mentioned this pull request Jun 14, 2019
Merged
32 tasks
@jasonrsadler jasonrsadler merged commit 68e1bd3 into master Jun 14, 2019
@jasonrsadler jasonrsadler deleted the reddit-inline branch June 14, 2019 13:25
@jasonrsadler jasonrsadler restored the reddit-inline branch June 14, 2019 13:32
@jasonrsadler jasonrsadler mentioned this pull request Jun 14, 2019
Merged
@jasonrsadler
Copy link
Contributor Author

Reopening for pending security review for brave/brave-core#2624

@jasonrsadler jasonrsadler mentioned this pull request Jun 14, 2019
Merged
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NejcZdovc NejcZdovc NejcZdovc left review comments

@ryanml ryanml ryanml approved these changes

Assignees

@jasonrsadler jasonrsadler

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

Successfully merging this pull request may close these issues.
3 participants
@jasonrsadler @ryanml @NejcZdovc