Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

replace yes/no to allow/deny for notifcation bar #10180

Merged
merged 1 commit into from
Aug 6, 2017
Merged

replace yes/no to allow/deny for notifcation bar #10180

merged 1 commit into from
Aug 6, 2017

Conversation

cezaraugusto
Copy link
Contributor


dear reviewer, do not land before #10178. thanks!


/cc @bradleyrichter pls see screenshots below. maybe we want to change strings for some questions too?

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Auditors: @luixxiul, @srirambv
close #9750

Test Plan:

notifications bar test should pass

npm run test -- --grep="notificationBar permissions"

Autoplay:

got to prefs->security and set autoplay to "always ask"
go to https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_video_autoplay
screen shot 2017-07-27 at 3 23 33 pm

Restart:

go to prefs->general and try to switch languages.

screen shot 2017-07-27 at 3 14 43 pm

Autofill:

go to prefs->security and set pw manager to built-in
go to any page that you have to write things so Brave can ask to save that for you
screen shot 2017-07-27 at 3 13 33 pm

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@cezaraugusto cezaraugusto added feature/notificationbar polish Nice to have — usually related to front-end/visual tasks. labels Jul 27, 2017
@cezaraugusto cezaraugusto added this to the 0.21.x Release milestone Jul 27, 2017
@cezaraugusto cezaraugusto self-assigned this Jul 27, 2017
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

lgtm thanks for the #10178 reminder

@srirambv
Copy link
Collaborator

Manually tested. Looks good to go

@bsclifton
Copy link
Member

@cezaraugusto can you rebase? then we're ready for merge 😄

@cezaraugusto
Copy link
Contributor Author

ok rebased waiting travis

@codecov-io
Copy link

codecov-io commented Aug 3, 2017

Codecov Report

Merging #10180 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10180      +/-   ##
==========================================
- Coverage   52.95%   52.94%   -0.01%     
==========================================
  Files         228      228              
  Lines       20308    20308              
  Branches     3254     3254              
==========================================
- Hits        10754    10753       -1     
- Misses       9554     9555       +1
Flag Coverage Δ
#unittest 52.94% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
app/browser/reducers/autoplayReducer.js 97.29% <ø> (ø) ⬆️
app/renderer/lib/extensionsUtil.js 58.88% <0%> (-1.56%) ⬇️
js/about/preferences.js 42.79% <0%> (+0.11%) ⬆️

@luixxiul
Copy link
Contributor

luixxiul commented Aug 3, 2017

isn't this ready to be merged? :-)

@luixxiul luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Aug 5, 2017
@cezaraugusto cezaraugusto removed the needs-info Another team member needs information from the PR/issue opener. label Aug 6, 2017
@cezaraugusto cezaraugusto merged commit 821b207 into master Aug 6, 2017
@cezaraugusto cezaraugusto deleted the 9750 branch August 6, 2017 23:06
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
@srirambv
Copy link
Collaborator

Somethings broken Seeing Yes/No for browser restart
image

@cezaraugusto
Copy link
Contributor Author

@srirambv what's wrong? should it be no/yes instead?

@cezaraugusto
Copy link
Contributor Author

actually for restart notification it should be yes/no (not sure about the order) but not allow/deny

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Dec 22, 2017 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature/notificationbar polish Nice to have — usually related to front-end/visual tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Yes/No buttons to Allow/Deny for easier understanding
8 participants