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

Note: text is displayed twice #17808

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Note: text is displayed twice #17808

merged 1 commit into from
Mar 31, 2023

Conversation

sujitacharya2005
Copy link
Contributor

@sujitacharya2005 sujitacharya2005 commented Mar 28, 2023 •
edited
Loading

Resolves brave/brave-browser#29081
Resolves brave/brave-browser#29080

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 wiki
    • npm run lint, npm run presubmit wiki, 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:

In this PR not resolved the Reward panel tool tip because it's new implementation will resolve this in rewards 3.0

In this PR resolves two issues

  1. Note: text is displayed twice
  2. Tipping banner text is updated based on creator wallettype connected and wallettype already verified
    https://user-images.githubusercontent.com/32419898/228364534-03f02471-5a1c-4ef1-a620-22727687ac3a.mp4

@sujitacharya2005 sujitacharya2005 added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x86 CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 labels Mar 28, 2023
deeppandya
deeppandya requested changes Mar 28, 2023
View reviewed changes
android/java/org/chromium/chrome/browser/rewards/BraveRewardsCreatorPanelFragment.java Outdated
&& !walletType.equals(BraveWalletProvider.UPHOLD)) {
notePart1 = String.format(
getString(R.string.brave_ui_site_banner_different_verified_notice_text1),
walletType, BraveWalletProvider.UPHOLD);
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like we have the same code for different else if parts. is there anything i am missing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes code is almost same except last parameter. like BraveWalletProvider.UPHOLD
R.string.brave_ui_site_banner_different_verified_notice_text1 this requires two parameters.
%1$s, %2$s
i.e.
Which custodian user is using and which custodian creator is verified.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can probably create a private function to get custodian name. that way we can avoid duplicated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@sujitacharya2005 sujitacharya2005 force-pushed the 29080_29080_Note_text_is_displayed_twice branch from 35d6195 to 0345978 Compare March 30, 2023 05:22
deeppandya
deeppandya requested changes Mar 30, 2023
View reviewed changes
android/java/org/chromium/chrome/browser/rewards/BraveRewardsCreatorPanelFragment.java Outdated
@@ -275,6 +275,26 @@ public void OnGetExternalWallet(String externalWallet) {
setPublisherNoteText(pubStatus, walletStatus);
}

private String getWalletString(String walletType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private String getWalletString(String walletType) {
private String getWalletStringFromType(String walletType) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

android/java/org/chromium/chrome/browser/rewards/BraveRewardsCreatorPanelFragment.java Outdated
}
}

private String getWalletString(int pubStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private String getWalletString(int pubStatus) {
private String getWalletStringFromStatus(int pubStatus) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@sujitacharya2005 sujitacharya2005 force-pushed the 29080_29080_Note_text_is_displayed_twice branch from 0345978 to ef4efbf Compare March 30, 2023 18:39
deeppandya
deeppandya approved these changes Mar 31, 2023
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 force-pushed the 29080_29080_Note_text_is_displayed_twice branch from ef4efbf to 66189d1 Compare March 31, 2023 11:56
@sujitacharya2005 sujitacharya2005 merged commit d9ec8cb into master Mar 31, 2023
@sujitacharya2005 sujitacharya2005 deleted the 29080_29080_Note_text_is_displayed_twice branch March 31, 2023 13:40
@github-actions github-actions bot added this to the 1.52.x - Nightly milestone Mar 31, 2023
brave-builds added a commit that referenced this pull request Apr 13, 2023
@brave-builds
Uplift of #17808 (squashed) to beta
3a05ede
@brave-builds brave-builds mentioned this pull request Apr 13, 2023
Merged
7 tasks
brave-builds added a commit that referenced this pull request Apr 13, 2023
@brave-builds
Uplift of #17808 (squashed) to release
b63d1af
@brave-builds brave-builds mentioned this pull request Apr 13, 2023
Merged
7 tasks
@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Apr 19, 2023 •
edited
Loading

Verification PASSED on Vivo X70 Pro version 12 running Bravemonoarm64.apk_1.52.56

Gemini pubs

BR panel Tipping banner
Screenshot_20230419_105842 Screenshot_20230419_105851

Bitflyer pubs

BR panel Tipping banner
Screenshot_20230419_110039 Screenshot_20230419_110055

@GeetaSarvadnya
Copy link

@sujitacharya2005 Cross custodial tipping notice is not shown in the BR panel in Android, but it's show in Desktop BR panel. Is this expected?

cc: @Miyayes @deeppandya

@sujitacharya2005
Copy link
Contributor Author

Hi @GeetaSarvadnya

In BR Panel showing cross custodian is not implemented. That is new thing, don't include in this Iteration. Let's keep a follow up issue. May be will include in rewards 3.0

@Miyayes WDYT?

@GeetaSarvadnya
Copy link

Thanks @sujitacharya2005. Logged a follow up issue brave/brave-browser#29845

kjozwiak pushed a commit that referenced this pull request Apr 19, 2023
@brave-builds
Note: text is displayed twice (uplift to 1.51.x) (#18054)
b97ec1f 
Uplift of #17808 (squashed) to beta
kjozwiak pushed a commit that referenced this pull request Apr 19, 2023
@brave-builds
Note: text is displayed twice (uplift to 1.50.x) (#18055)
0a2b9d9 
Uplift of #17808 (squashed) to release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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 CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64
Projects
None yet
Milestone
1.52.x - Release
3 participants
@sujitacharya2005 @GeetaSarvadnya @deeppandya