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.

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

New Tab Page WebUI: automatically dismiss branded wallpaper notifications… #4457

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

petemill
Copy link
Member

@petemill petemill commented Jan 28, 2020

… after page has been viewed for 4 seconds

Fix brave/brave-browser#7945

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@petemill petemill self-assigned this Jan 28, 2020
@petemill petemill force-pushed the ntp-sbw-notification-auto-dismiss branch 2 times, most recently from 9c8e486 to ab391ad Compare January 29, 2020 04:49
…ions after page has been viewed for 4 seconds
* Waits until the viewport has been in view for a certain number of seconds
* continuously before calling a provided function.
*/
export default class VisibilityTimer {
Copy link
Member

Choose a reason for hiding this comment

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

Really nice snippet of code here 😄👌

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Really nice work here! Verified the test plan (including 4 seconds required view time). Code changes look good. In a perfect world, it would be amazing to have a browser test for this (or the branded NTP in general). If you think that might be doable (in the future of course!), could you file an issue with the requirements for a test?

@bsclifton
Copy link
Member

CI passed (all green!) - going to merge so we can get this into the next Nightly 😄

@bsclifton bsclifton merged commit 3eb234f into master Jan 29, 2020
@bsclifton bsclifton deleted the ntp-sbw-notification-auto-dismiss branch January 29, 2020 07:51
petemill pushed a commit that referenced this pull request Jan 29, 2020
New Tab Page WebUI: automatically dismiss branded wallpaper notifications…
This was referenced Jan 29, 2020
petemill pushed a commit that referenced this pull request Jan 29, 2020
New Tab Page WebUI: automatically dismiss branded wallpaper notifications…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NTP SI messages should only be shown the first time a NTP SI is shown
2 participants