Skip to content
This repository has been archived by the owner. It is now read-only.

/ browser-laptop Archived

Show search indicator when urlBar is empty #8491

Merged
merged 1 commit into from May 4, 2017
Merged

Show search indicator when urlBar is empty #8491

merged 1 commit into from May 4, 2017

Conversation

@cezaraugusto
Copy link
Contributor

cezaraugusto commented Apr 25, 2017 •
edited by 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).

Close #8468

Test Plan:
covered by automated test

QA Steps:
empty url bar should have search icon instead of current one
@cezaraugusto cezaraugusto added this to the 0.15.1 milestone Apr 25, 2017
@cezaraugusto cezaraugusto self-assigned this Apr 25, 2017
@cezaraugusto cezaraugusto requested review from bsclifton and NejcZdovc Apr 25, 2017
@cezaraugusto cezaraugusto changed the title - Auditors: @bsclifton, @nejczdovc Show search indicator when urlBar is empty Apr 25, 2017
@srirambv srirambv mentioned this pull request Apr 26, 2017
Closed
@bsclifton
bsclifton reviewed May 1, 2017
View changes
Copy link
Member

bsclifton left a comment

On the before (current master), I can't reproduce the issue where the search icon isn't showing. Do you have steps?
The steps shown in #8468 work fine. ex: select URL,do cmd + x for cut, notice search icon is shown (magnifying glass)
@bsclifton
bsclifton reviewed May 1, 2017
View changes
Copy link
Member

bsclifton left a comment •
edited

using the GIF attached to #8468, I was able to repro (keyboard immediately sets the magnifying glass; right click, shortcut does not)... but the problem is, the issue still happens (even with this patch)
urlbar
@cezaraugusto
Show search indicator when urlBar is empty
Verified
This commit was signed with a verified signature.
cezaraugusto Cezar Augusto
GPG key ID: ECC5FF1C7EA1D39D Learn about signing commits
Loading status checks…
53919e6 
- Auditors: @bsclifton, @NejcZdovc
- Close #8468
- Test Plan: covered by automated test
@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented May 3, 2017

my bad I misunderstand STR. Updated
@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented May 3, 2017

btw side-effect is that this brings back search indicator when urlbar is focused, described in this comment: https://github.com/brave/browser-laptop/blob/master/app/renderer/components/navigation/urlBarIcon.js#L45
@bsclifton
bsclifton approved these changes May 4, 2017
View changes
Copy link
Member

bsclifton left a comment

Works perfect! ++
@bsclifton bsclifton merged commit 8ed5793 into brave:master May 4, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@cezaraugusto cezaraugusto deleted the cezaraugusto:urlbar/8468 branch Jul 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bsclifton bsclifton

@NejcZdovc NejcZdovc

Assignees

@cezaraugusto cezaraugusto

Labels
QA/test-plan-specified feature/URLbar parity
Projects
None yet
Milestone
  0.15.300 (Frozen,...
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@cezaraugusto @bsclifton
You can’t perform that action at this time.