convert toolbar icons to svg

[jkup]

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

Fix #4852 #5210

Auditors @bsclifton @bbondy @bradleyrichter

Things needed to ship:

• back and forward buttons need an svg for hover state
• verified svg is missing from zip
• design review from @bradleyrichter :)

After going over it with @bradleyrichter here is how it currently looks:

Also here is the windows version:

Testing:

The 5 possible buttons to the left of the urlbar have all been replaced. This includes back, forward, stop, reload and home.

1. Test back / forward functionality. They should both work as before and now the color should not change but the button's background should turn white and have a small shadow.
2. Test reload and stop. Functionality should be the same as before but they should now have the same hover effect as back / forward.
3. In preferences > general enable home page button and make sure it looks well with the others and has the same hover style.
4. After visiting a verified publisher (such as clifton.io) go to Preferences > Payments and make sure the new verified icon (a green hexagon with a white checkmark) shows up and looks good.

[bradleyrichter]

That should work. We will need a hover state for all buttons. Shall we use the same approach?

[jkup]

Only if there is a background color change which I think is back and forward buttons only

[jkup]

@bsclifton I mentioned to you that we could get the backbutton / forwardbutton hover effect working with just a single image. I implemented that but would love it if you'd take a look at the approach (https://github.com/brave/browser-laptop/pull/5173/files#diff-303f0b6a297506f2cc18bf7b4cb370c5R912) If this approach works we can ship without additional SVGs.

Also, for the life of me I can't figure out why on my branch the entire center navigation is pushed slightly to the right. If anyone could help me figure that out it would be greatly appreciated!

[bsclifton]

Checking this out now...

[bsclifton]

Great job! 😄 For folks following along, here's a screen cap of what it looks like on hover:

I did notice this though, with the navigationButton class; it doesn't fill the parent div entirely, so you can put the mouse over and it won't apply the hover style (compare to the above shot):

Here's a shot where you can see the bounds:

[bsclifton]

@jkup for the offset issue, you're gonna wanna check here:

browser-laptop/less/navigationBar.less

Line 33 in 05fbdc1

@centerOffset: @navbarLeftMarginDarwin + 2 * (@navbarButtonWidth + @navbarButtonSpacing) // width area on the left

The nav button sizes are stored in variables.less and this centerOffset uses those for the calculation. Hope this helps 😄

[bsclifton]

Here's a shot on Windows- the star icon and the element surrounding it seems to have an issue:

Otherwise looks great 👍 (besides the parts not finished yet, which are still WIP)

[bradleyrichter]

@jkup This is looking great. The main thing missing is to reduce the size of the "X" (stop loading) button icon. I think about 30% smaller will be good.

[jkup]

@bradleyrichter reduced the size! still need to make sure it looks good on Windows before shipping.

[bsclifton]

Couple of small things (doh) - I checked in some changed fixing these items (see my commit)

dropdown under back / forward

When you have a few things in your back/forward cache and you do a long press, the drop down isn't positioned correctly

I fixed this by passing in the target (instead of the rect) from our custom LongPressButton control and then doing the target.parentNode trick (like you did for the hamburger menu). Here's the fixed one:

new tab button

is this how it should look on mouse hover?

Assuming that's right, the position seems to be off:

I made some tweaks to it and here's how it looks now:

hamburger

hamburger had 2 pixels gap (was needed with the old hamburger icon, but not this)-

Removed that so it matches the current look (before element was resized to for ellipsis)

also updated cursor to be default; if you click and drag in that area, it would show the text input as your cursor (weird).

[bsclifton]

@jkup let me know when this is ready for re-review 😄

[jkup]

@bsclifton meant to update you -- should be ready for re-review. I could go either way on the position on the buttons on Windows. If you'd like them bumped up 1 px let me know but I think they look good!

[bradleyrichter]

Mac looks great but still needs a reduction for the stop icon.

Windows needs a 1px nudge up for the bookmark star and the actual URL text string.

On Nov 6, 2016, at 11:43 AM, Jon Kuperman notifications@github.com wrote:

@bsclifton meant to update you -- should be ready for re-review. I could go either way on the position on the buttons on Windows. If you'd like them bumped up 1 px let me know but I think they look good!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[jkup]

@bradleyrichter fixed and fixed

[bsclifton]

Checking this out right now... 😄

[bsclifton]

Checked in some small tweaks after testing locally (otherwise great!)

• move star up 1px
• make reload/stop/home the same size (on hover) as back/forward
  • move reload/stop/home out of startButtons and into navigator
  • change reload/stop/home to pos relative, move down 3 px

Here's a screencap showing the hover state for reload/stop being the same size as back/forward

Here's a screenshot showing the whole thing on Windows

[bsclifton]

Looks good to me! 😄

@bbondy before I merge, did you want to cherry pick the commits and give it a spin? I screen capped the buttons and zoomed in, measured the pixels on each (on Windows + Mac). I had tweaks (see commits) and it should be good to go now

[bbondy]

I'll merge and see it on master, thanks!