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

Move NoScript icon into URL bar. This fixes draggability dead area #8236

Merged
merged 1 commit into from
Apr 11, 2017
Merged

Move NoScript icon into URL bar. This fixes draggability dead area #8236

merged 1 commit into from
Apr 11, 2017

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Apr 11, 2017 •
edited
Loading

Test plan

  1. Make sure the non-draggable area specified in Added noscript into the URL bar - Move noscript into the URL bar #5792 is draggable
  2. Make sure every button on the navigation bar is clickable
  3. Open https://jsfiddle.net/
  4. Block scripts from the top right lion icon
  5. Make sure the noScript button on the navigation bar is clickable

Description

Fixes #5792

Auditors: @bradleyrichter, @luixxiul

  • 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).

screen shot 2017-04-11 at 11 51 08 am

@bsclifton bsclifton added this to the 0.14.2 milestone Apr 11, 2017
@bsclifton bsclifton self-assigned this Apr 11, 2017
luixxiul
luixxiul suggested changes Apr 11, 2017
View reviewed changes
Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

On normal pages it works great, though on about pages you could not drag the window.

@Jacalz
Copy link
Contributor

Jacalz commented Apr 11, 2017 •
edited
Loading

This looks great, have never liked the old no scrip icon 👍 Great job 👍

@Jacalz
Copy link
Contributor

Jacalz commented Apr 11, 2017

Will add this to my test for the next version of 0.14.2 👍

@bsclifton
Copy link
Member Author

Great catch, @luixxiul! I just pushed a fix, can you try it?

@diracdeltas
Copy link
Member

diracdeltas commented Apr 11, 2017 •
edited
Loading

some
noscript-related tests are failing: https://travis-ci.org/brave/browser-laptop/jobs/221100103#L4823

@bsclifton
Copy link
Member Author

@diracdeltas good catch! 😄 Thanks- will check those out

bradleyrichter
bradleyrichter reviewed Apr 11, 2017
View reviewed changes
Copy link
Contributor

@bradleyrichter bradleyrichter left a comment

Choose a reason for hiding this comment

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

I approve this icon size and positioning. TY!

@bsclifton
Move NoScript icon into URL bar. This fixes draggability dead area th…
d01daf4 
…at was present

Fixes #5792

Auditors: @bradleyrichter, @luixxiul

Includes: Fix noscript tests
Auditors: @diracdeltas
@bsclifton
Copy link
Member Author

@diracdeltas fixed! 😄

@bsclifton
Copy link
Member Author

NoScript tests passed- merging!

@bsclifton bsclifton merged commit fafe10e into master Apr 11, 2017
@bsclifton bsclifton deleted the fix-noscript-icon branch April 11, 2017 22:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradleyrichter bradleyrichter bradleyrichter left review comments

@luixxiul luixxiul luixxiul requested changes

Assignees

@bsclifton bsclifton

Labels
feature/URLbar
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.
5 participants
@bsclifton @Jacalz @diracdeltas @luixxiul @bradleyrichter