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.

Sign up for GitHub

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

Jump to bottom

Add rewards parity with browser laptop #241

Merged
merged 84 commits into from
Sep 4, 2018
Merged

Add rewards parity with browser laptop #241

merged 84 commits into from
Sep 4, 2018

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jul 11, 2018 •
edited
Loading

@NejcZdovc NejcZdovc self-assigned this Jul 11, 2018
bridiver
bridiver reviewed Jul 11, 2018
View reviewed changes
components/brave_rewards_ui/components/settings.js Outdated
@@ -0,0 +1,336 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Collaborator

Choose a reason for hiding this comment

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

how is this file used? I don't see it added to a gn file anywhere

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is probably supposed to go in resources/BUILD.gn

Copy link
Contributor Author

@NejcZdovc NejcZdovc Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

this is called from components/brave_rewards_ui/components/app.js

Copy link
Collaborator

Choose a reason for hiding this comment

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

then it needs to be added to resources/BUILD.gn or changes will not trigger webpack to rebuild

Copy link
Collaborator

Choose a reason for hiding this comment

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

every input file should always be added to resources/BUILD.gn even if it is included by another file because the build system doesn't understand js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sounds good will add it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's not needed, because every brave_*_ui folder provides only one file via webpack. Let me know if it's still needed

bridiver
bridiver reviewed Jul 11, 2018
View reviewed changes
browser/ui/webui/brave_webui_source.cc Outdated
@@ -81,6 +81,11 @@ void CustomizeWebUIHTMLSource(const std::string &name, content::WebUIDataSource*
}
}, {
std::string("rewards"), {
{ "823882f2419bf6c16b8291e056ea4b52.svg", IDR_BRAVE_REWARDS_IMG_WALLET_ICON },
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like these images are being included by webpack so I don't think we need this

Copy link
Collaborator

@bridiver bridiver Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I guess I'm not clear on what is happening with the images in webpack so maybe we need them. Did you already try without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we need to add it otherwise they are not found

Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be nice if webpack could bundle these somehow

bridiver
bridiver reviewed Jul 11, 2018
View reviewed changes
package-lock.json
@@ -1,6 +1,6 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this change is supposed to be here. @bbondy suggested that you might have an older version of npm. I get the same thing when I run init so I'm checking mine as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it needs to be here, because I updated brave-ui package

@bbondy bbondy changed the title Adds rewards settings WIP: Adds rewards settings Jul 12, 2018
@NejcZdovc NejcZdovc force-pushed the rewards branch 3 times, most recently from f70a592 to d797b58 Compare July 17, 2018 07:35
@NejcZdovc NejcZdovc force-pushed the rewards branch 2 times, most recently from 8f50a22 to e513fbe Compare July 21, 2018 06:11
@NejcZdovc NejcZdovc force-pushed the rewards branch 3 times, most recently from 9359e34 to 8c23d63 Compare August 1, 2018 05:30
@NejcZdovc NejcZdovc mentioned this pull request Aug 1, 2018
Closed
9 tasks
@NejcZdovc NejcZdovc changed the title WIP: Adds rewards settings WIP: Add rewards settings Aug 1, 2018
@NejcZdovc NejcZdovc force-pushed the rewards branch 3 times, most recently from b1f1ab1 to 0284134 Compare August 1, 2018 17:18
@NejcZdovc NejcZdovc changed the title WIP: Add rewards settings WIP: Add rewards Aug 8, 2018
@NejcZdovc NejcZdovc force-pushed the rewards branch 6 times, most recently from 9a6dae4 to 3b98b36 Compare August 16, 2018 18:07
@NejcZdovc NejcZdovc changed the title WIP: Add rewards parity with browser laptop Add rewards parity with browser laptop Sep 3, 2018
jasonrsadler
jasonrsadler reviewed Sep 3, 2018
View reviewed changes
browser/ui/webui/brave_rewards_ui.cc Outdated

// PaymentServiceObserver implementation
void OnWalletCreated(payments::PaymentsService* payment_service,
void OnWalletInitialized(brave_rewards::RewardsService* payment_service,
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 payment_service to be rewards_service?

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

jasonrsadler
jasonrsadler reviewed Sep 3, 2018
View reviewed changes
browser/ui/webui/brave_rewards_ui.cc Outdated
}

void RewardsDOMHandler::OnWalletInitialized(
brave_rewards::RewardsService* payment_service,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here?

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

jasonrsadler
jasonrsadler reviewed Sep 3, 2018
View reviewed changes
components/brave_rewards/browser/rewards_helper.cc
@@ -21,9 +21,11 @@

using content::ResourceType;

namespace payments {
// DEFINE_WEB_CONTENTS_USER_DATA_KEY(brave_rewards::RewardsHelper);
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 for future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @bridiver

jasonrsadler
jasonrsadler reviewed Sep 3, 2018
View reviewed changes
components/brave_rewards/browser/rewards_helper.h
@@ -2,8 +2,8 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_BROWSER_PAYMENTS_PAYMENTS_HELPER_
#define BRAVE_BROWSER_PAYMENTS_PAYMENTS_HELPER_
#ifndef BRAVE_BROWSER_BRAVE_REWARDS_REWARDS_HELPER_
Copy link
Contributor

Choose a reason for hiding this comment

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

Linter will complain here once set. Should be:
BRAVE_BROWSER_BRAVE_REWARDS_BROWSER_REWARDS_HELPER_

@NejcZdovc NejcZdovc force-pushed the rewards branch 3 times, most recently from 970420d to 3162763 Compare September 4, 2018 08:13
SergeyZhukovsky
SergeyZhukovsky approved these changes Sep 4, 2018
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++.
I almost reviewed all of that during the implementation. I just checked commits that I missed. Looks good

@NejcZdovc NejcZdovc merged commit 24688e1 into master Sep 4, 2018
@NejcZdovc NejcZdovc mentioned this pull request Sep 4, 2018
Closed
cezaraugusto pushed a commit that referenced this pull request Jul 17, 2019
@ryanml
Imported from brave-ui: Merge pull request #241 from ryanml/feature-223
afc7574 
Adds new 2 state tab component
@NejcZdovc NejcZdovc modified the milestones: Closed, 0.55.x - Release Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonrsadler jasonrsadler jasonrsadler left review comments

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@bridiver bridiver Awaiting requested review from bridiver bridiver is a code owner

@bbondy bbondy Awaiting requested review from bbondy

@ryanml ryanml Awaiting requested review from ryanml

Assignees

@NejcZdovc NejcZdovc

Labels
feature/rewards
Projects
None yet
Milestone
0.55.x - Release
Development

Successfully merging this pull request may close these issues.

UGP support Support Brave Payment features about:rewards
4 participants
@NejcZdovc @bridiver @SergeyZhukovsky @jasonrsadler