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

Ads fixes for Android Ads Custom Notifications #7293

Merged
merged 4 commits into from
May 19, 2021
Merged

Ads fixes for Android Ads Custom Notifications #7293

merged 4 commits into from
May 19, 2021

Conversation

yachtcaptain23
Copy link
Contributor

@yachtcaptain23 yachtcaptain23 commented Dec 1, 2020 •
edited
Loading

swipe_up_ad

Closes brave/brave-browser#13038

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
  • 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:

On Android, go through OOBE and observe the new push notification test. Try swiping up to dismiss and make sure tapping opens up the notification.

@tmancey tmancey self-requested a review December 1, 2020 10:44
tmancey
tmancey approved these changes Dec 1, 2020
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM

@kjozwiak kjozwiak mentioned this pull request Feb 8, 2021
Closed
@yachtcaptain23 yachtcaptain23 requested review from kylehickinson and a team as code owners April 19, 2021 22:25
@yachtcaptain23 yachtcaptain23 marked this pull request as draft April 19, 2021 22:26
@yachtcaptain23 yachtcaptain23 marked this pull request as ready for review April 20, 2021 18:36
@kylehickinson kylehickinson removed their request for review April 20, 2021 18:55
@yachtcaptain23
Ads fixes:
be42315 
(1) Font size and background
(2) Fix buttons to use 48dp

Support dismiss instead of X.

Closes brave/brave-browser#12956
@yachtcaptain23
Better drag behavior
7533fb1
samartnik
samartnik reviewed May 18, 2021
View reviewed changes
...r/brave_ads/android/java/org/chromium/chrome/browser/dialogs/BraveAdsNotificationDialog.java Outdated
@@ -70,44 +81,73 @@ public static void showAdNotification(Context context, final String notification
window.setFlags(WindowManager.LayoutParams.FLAG_NOT_TOUCH_MODAL,
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 still need this call given that flag had been set before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

...r/brave_ads/android/java/org/chromium/chrome/browser/dialogs/BraveAdsNotificationDialog.java Outdated
BraveAdsNativeHelper.nativeOnCloseAdNotification(
Profile.getLastUsedRegularProfile(), mNotificationId, true);
mNotificationId = null;
public boolean onTouch(View v, MotionEvent event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we handling swipes up and down? Usually notifications are dismissed by swiping left and right on Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ads team wanted to copy the behavior from iOS. @tmancey would you chime in? I don't know if I can make it in time for trains, but we can let it sit and if users want left/right swipe I can do so.

Copy link
Collaborator

@tmancey tmancey May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@jenn-rhim Your thoughts? For me, I like to follow the system's default, which as above is left/right. @yachtcaptain23 Depending on the decision from @jenn-rhim we should make the change before merging. Thanks

...r/brave_ads/android/java/org/chromium/chrome/browser/dialogs/BraveAdsNotificationDialog.java Outdated
case MotionEvent.ACTION_MOVE:
deltaY = event.getRawY() + mYDown;
if (deltaY > 0) {
deltaY = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@tmancey
Copy link
Collaborator

tmancey commented May 18, 2021

@yachtcaptain23 Can you please run npm run lint as linter is failing, thanks

@tmancey tmancey requested review from tmancey and samartnik May 18, 2021 16:22
samartnik
samartnik approved these changes May 18, 2021
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, the only concern if the direction of swiping

@tmancey
Copy link
Collaborator

tmancey commented May 18, 2021

@samartnik Discussed with @jenn-rhim and swipe up is preferred. Thanks

tmancey
tmancey approved these changes May 19, 2021
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM

@tmancey
Copy link
Collaborator

tmancey commented May 19, 2021

CI failed for unrelated JUnit tests

@tmancey tmancey merged commit d1871bc into master May 19, 2021
@tmancey tmancey deleted the issues/12956 branch May 19, 2021 14:40
@tmancey tmancey added this to the 1.26.x - Nightly milestone May 19, 2021
@GeetaSarvadnya GeetaSarvadnya mentioned this pull request Jun 11, 2021
Closed
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

@tmancey tmancey tmancey approved these changes

@SergeyZhukovsky SergeyZhukovsky Awaiting requested review from SergeyZhukovsky

Assignees

@yachtcaptain23 yachtcaptain23

Labels
None yet
Projects
None yet
Milestone
1.26.x - Release
Development

Successfully merging this pull request may close these issues.

Implement swipe-to-dismiss for Brave Ads custom ad notifications
3 participants
@yachtcaptain23 @tmancey @samartnik