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

Show 'Brave Browser' as the origin if origin is null #10668

Merged
merged 1 commit into from
Aug 30, 2017
Merged

Show 'Brave Browser' as the origin if origin is null #10668

merged 1 commit into from
Aug 30, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Aug 25, 2017 •
edited
Loading

in the permission notification bar

fix #4198

Test Plan:

  1. go to brave.com
  2. scroll down to the contact section
  3. right click on one of the email addresses to open in a new tab
  4. the notification bar should say, 'Allow Brave Browser to open an external application?'

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.

Test Plan:

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

@diracdeltas
Show 'Brave Browser' as the origin if origin is null
3383ff6 
in the permission notification bar

fix #4198

Test Plan:
1. go to brave.com
2. scroll down to the contact section
4. right click on one of the email addresses to open in a new tab
5. the notification bar should say, 'Allow Brave Browser to open an external application?'
@diracdeltas diracdeltas added this to the 0.21.x (Nightly Channel) milestone Aug 25, 2017
@diracdeltas diracdeltas self-assigned this Aug 25, 2017
@diracdeltas diracdeltas requested a review from kjozwiak August 25, 2017 22:17
@codecov-io
Copy link

codecov-io commented Aug 25, 2017 •
edited
Loading

Codecov Report

Merging #10668 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #10668      +/-   ##
==========================================
- Coverage   54.54%   54.53%   -0.01%     
==========================================
  Files         245      245              
  Lines       21156    21157       +1     
  Branches     3263     3264       +1     
==========================================
  Hits        11539    11539              
- Misses       9617     9618       +1
Flag Coverage Δ
#unittest 54.53% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
app/filtering.js 18.19% <0%> (-0.04%) ⬇️

@diracdeltas diracdeltas requested a review from bsclifton August 28, 2017 23:18
kjozwiak
kjozwiak approved these changes Aug 30, 2017
View reviewed changes
Copy link
Member

@kjozwiak kjozwiak left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell. I also pulled the PR and went through a few manual tests and made sure the change was working correctly.

@diracdeltas diracdeltas merged commit ee9f5ee into master Aug 30, 2017
@diracdeltas diracdeltas deleted the fix/4198 branch August 30, 2017 02:39
@Sh1d0w Sh1d0w mentioned this pull request Aug 30, 2017
Closed
@srirambv
Copy link
Collaborator

No notification on Windows
4198

@luixxiul
Copy link
Contributor

luixxiul commented Sep 21, 2017 •
edited
Loading

@srirambv I think it is because it was not merged to 0.19.x nvm I found this: #4198 (comment)

@luixxiul luixxiul modified the milestones: 0.21.x (Nightly Channel), 0.19.x (Beta Channel) Sep 21, 2017
@luixxiul
Copy link
Contributor

@diracdeltas I got instead Allow https://brave.com to open an external application. Is it ok?

@luixxiul luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Sep 23, 2017
@diracdeltas
Copy link
Member Author

@luixxiul i think it's fine as long as it's not null

@luixxiul luixxiul removed the needs-info Another team member needs information from the PR/issue opener. label Sep 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kjozwiak kjozwiak kjozwiak approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@diracdeltas diracdeltas

Labels
None yet
Projects
None yet
Milestone
0.19.x (Beta Channel)
Development

Successfully merging this pull request may close these issues.

mailto: notifications display "null" rather than the domain when opening in new tabs
5 participants
@diracdeltas @codecov-io @srirambv @luixxiul @kjozwiak