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

New tipping banner UI brave-browser/issues/17070 #14967

Merged
merged 39 commits into from
Sep 27, 2022
Merged

New tipping banner UI brave-browser/issues/17070 #14967

merged 39 commits into from
Sep 27, 2022

Conversation

sujitacharya2005
Copy link
Contributor

@sujitacharya2005 sujitacharya2005 commented Sep 2, 2022 •
edited by SergeyZhukovsky
Loading

Resolves brave/brave-browser#17070
Resolves brave/brave-browser#16021

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Verified with out publisher description Monthly tip type selected Verified with publisher description available Unverified publisher
Screenshot_20220921_152237 Screenshot_20220921_152357 Screenshot_20220921_152441 Screenshot_20220921_152533
Custom Tip Selected Custom Tip Confirmation One time confirmation monthly tip confirmation
Screenshot_20220921_152607 Screenshot_20220921_152627 Screenshot_20220921_152642 Screenshot_20220921_152714
Tip error case
Screenshot_20220921_161536

@sujitacharya2005 sujitacharya2005 added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Sep 2, 2022
@sujitacharya2005 sujitacharya2005 requested a review from a team as a code owner September 2, 2022 14:31
samartnik
samartnik requested changes Sep 7, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/rewards/BraveRewardsBannerInfo.java Outdated Show resolved Hide resolved
android/java/org/chromium/chrome/browser/rewards/BraveRewardsBannerInfo.java Outdated
public static final String DESCRIPTION = "description";
public static final String BACKGROUND = "background";
public static final String LOGO = "logo";
// public static final String AMOUNTS = "amounts";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented, will we need it going forward?

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

browser/brave_rewards/android/brave_rewards_native_worker.cc Outdated Show resolved Hide resolved
@sujitacharya2005 sujitacharya2005 force-pushed the Fix_for_17070_Tipping_banner1 branch 2 times, most recently from 8ea7cbb to e0c487b Compare September 7, 2022 19:09
@sujitacharya2005 sujitacharya2005 marked this pull request as draft September 8, 2022 10:17
samartnik
samartnik reviewed Sep 22, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/BraveRewardsSiteBannerActivity.java Show resolved Hide resolved
android/java/org/chromium/chrome/browser/rewards/BraveRewardsTipConfirmationFragment.java Show resolved Hide resolved
android/java/org/chromium/chrome/browser/rewards/BraveRewardsCreatorPanelFragment.java Outdated
Comment on lines 313 to 314
Log.e("BraveRewards",
"User is verified and publisher is verified but not with the same provider");
Copy link
Contributor

@samartnik samartnik Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If this is not common case (happens rarely) I would probably keep it as it won't clutter the log, but may help with debugging this issue.
What I would do though is replace BraveRewards with TAG variable for code consistency with Chromium and less likely to make a typo in tag and have problems with filtering it in the LogCat.

android/java/org/chromium/chrome/browser/rewards/BraveRewardsCreatorPanelFragment.java Show resolved Hide resolved
android/java/org/chromium/chrome/browser/rewards/BraveRewardsCreatorPanelFragment.java Outdated
new BraveRewardsExternalWallet(externalWallet);
walletStatus = mExternalWallet.getStatus();
} catch (JSONException e) {
Log.e("BraveRewards", e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add assert, it will be easier to catch it

samartnik
samartnik approved these changes Sep 23, 2022
View reviewed changes
Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

lgtm

android/java/org/chromium/chrome/browser/rewards/BraveRewardsCreatorPanelFragment.java Show resolved Hide resolved
SergeyZhukovsky
SergeyZhukovsky approved these changes Sep 23, 2022
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.

++

deeppandya
deeppandya reviewed Sep 26, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/BraveRewardsAmountChangeListener.java Outdated
@@ -0,0 +1,5 @@
package org.chromium.chrome.browser.rewards;
Copy link
Contributor

Choose a reason for hiding this comment

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

We would need copyright section here.
`/** Copyright (c) 2022 The Brave Authors. All rights reserved.

  • This Source Code Form is subject to the terms of the Mozilla Public
  • 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/.
    */`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This extra file deleted

android/java/org/chromium/chrome/browser/rewards/BraveRewardsAmountChangeListener.java

package org.chromium.chrome.browser.rewards;

public interface BraveRewardsAmountChangeListener {
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 have BraveRewardsAmountChangeListener at two different places ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This extra file deleted

deeppandya
deeppandya reviewed Sep 26, 2022
View reviewed changes
android/java/res/layout/brave_rewards_tip_confirmation.xml Outdated
android:fontFamily="@font/poppins_medium"
android:text="@string/thanks_for_support"
android:textStyle="bold"
android:textColor="#212529"
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 avoid using hard coded color codes. i think we have this color code for text for other parts

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 all hard coded color code

deeppandya
deeppandya reviewed Sep 26, 2022
View reviewed changes
android/java/res/layout/brave_rewards_tip_confirmation.xml Outdated
android:layout_marginStart="84dp"
android:layout_marginTop="9.85dp"
android:layout_marginEnd="84dp"
android:background="#80AEB1C2"
Copy link
Contributor

@deeppandya deeppandya Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

same as other comment

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 all hard coded color code

deeppandya
deeppandya reviewed Sep 26, 2022
View reviewed changes
android/java/res/layout/brave_rewards_custom_tip_fragment.xml
android:background="@android:color/white"
android:paddingTop="8dp"
>

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space

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

deeppandya
deeppandya approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

LGTM

@sujitacharya2005 sujitacharya2005 merged commit fc55a8f into master Sep 27, 2022
@sujitacharya2005 sujitacharya2005 deleted the Fix_for_17070_Tipping_banner1 branch September 27, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samartnik samartnik samartnik approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@Pavneet-Sing Pavneet-Sing Pavneet-Sing approved these changes

@deeppandya deeppandya deeppandya approved these changes

Assignees

@sujitacharya2005 sujitacharya2005

Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Milestone
1.46.x - Release
Development

Successfully merging this pull request may close these issues.

[Android] Implement new tipping banner (Phase 1) [Android] Add Custom Tipping Amount feature (Phase 1)
5 participants
@sujitacharya2005 @deeppandya @Pavneet-Sing @SergeyZhukovsky @samartnik