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

Add new "empty new tab" option in preferences (for what new tab does) #5690

Merged
merged 3 commits into from
Nov 22, 2016
Merged

Add new "empty new tab" option in preferences (for what new tab does) #5690

merged 3 commits into from
Nov 22, 2016

Conversation

bsclifton
Copy link
Member

  • 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 2016-11-16 at 12 13 47 pm

includes tests 😄

Fixes #5682

Please note: choosing this option does NOT put focus in the URL bar. Is this OK?
cc: @diracdeltas

Auditors: @bradleyrichter, @bbondy

Test Plan:

  1. Launch Brave and go to preferences
  2. Change "A new tab shows" to "empty new tab"
  3. Open a new tab; notice it's about:blank

@bradleyrichter
Copy link
Contributor

@bsclifton I'll pull this in and edit the strings and commit those.

@bsclifton
Copy link
Member Author

Working with @bradleyrichter on this one... not ready to be accepted yet

@bradleyrichter
Copy link
Contributor

current proposal:

image

@bbondy
Copy link
Member

bbondy commented Nov 17, 2016

Please note: choosing this option does NOT put focus in the URL bar. Is this OK?

No it should still react like a new tab page and put the focus in the URL bar. I think this option should modify how about:newtab works to not render anything (return null) and not use about:blank.

I'm going to move it to 0.12.11.

@bbondy bbondy modified the milestones: 0.12.11, 0.12.10 release Nov 17, 2016
@bsclifton
Copy link
Member Author

Awesome- I think it makes sense pushing this back a release to make sure it's solid. We hashed out some ideas on Slack and it would be good not to rush it 😄

@luixxiul
Copy link
Contributor

@bradleyrichter what is "My dashboard" there?

@bradleyrichter
Copy link
Contributor

Dashboard is the new proposed name for the thing otherwise know as the new tab page.

On Nov 16, 2016, at 8:33 PM, Suguru Hirahara notifications@github.com wrote:

@bradleyrichter what is "My dashboard" there?


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

@bsclifton bsclifton changed the title Add new "empty new tab" option in preferences (for what new tab does) [WIP] Add new "empty new tab" option in preferences (for what new tab does) Nov 18, 2016
@bsclifton bsclifton changed the title [WIP] Add new "empty new tab" option in preferences (for what new tab does) Add new "empty new tab" option in preferences (for what new tab does) Nov 21, 2016
@bsclifton
Copy link
Member Author

bsclifton commented Nov 21, 2016

@bbondy ready for re-review 😄

Here's what the choice dropdown looks like:
screen shot 2016-11-21 at 12 18 50 am

Things to note:

  • I updated the only occurrence of "homepage" to "home page" (see screenshot)
  • Regardless of whether you pick Blank page or Dashboard, you go to about:newtab. This means the URL bar is focused
  • new webdriver test created for blank about:newtab check

@bsclifton
Copy link
Member Author

Rebased, resolved conflicts with tests, and made sure tests pass great 😄 Ready for re-review, @bbondy

Picture as proof that I'm not crazy 😄
screen shot 2016-11-21 at 11 00 23 pm

@bbondy
Copy link
Member

bbondy commented Nov 22, 2016

looks like app/extensions/brave/locales/en-US/preferences.properties has a conflict, could you rebase?

@bbondy
Copy link
Member

bbondy commented Nov 22, 2016

Pls also run through automated if you don't mind, review of code looks good.

bsclifton and others added 3 commits November 22, 2016 09:53
*includes tests* :)

Fixes #5682

Please note: choosing this option does NOT put focus in the URL bar. Is this OK?
cc: @diracdeltas

Auditors: @bradleyrichter, @bbondy

Test Plan:
1. Launch Brave and go to preferences
2. Change "A new tab shows" to "empty new tab"
3. Open a new tab; notice it's about:blank
- if empty tab, about:newtab returns an empty div
- regardless of choice, page is about:newtab (not about:blank)
- tests added to reinforce this behavior

Auditors: @bbondy, @cezaraugusto
@bbondy
Copy link
Member

bbondy commented Nov 22, 2016

++ thanks!

@bbondy bbondy merged commit 5d86ccd into brave:master Nov 22, 2016
@bsclifton
Copy link
Member Author

@bbondy thanks for checking this out and merging 😊

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants