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

Multiple home page URL #6922

Merged
merged 1 commit into from
Feb 1, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jan 30, 2017

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

Description

Resolves #6913

This problem was introduced with c44b0eb, because {} was added as default value to frameOpts.

Auditors

@bsclifton, @bradleyrichter

Test Plan

1 plan:

  • set home page url with multiple url's value for example file:///index.htmlhttps://start.duckduckgo.com/|https://google.com/
  • restart browser and home page URL's should be opened correctly

2 plan:

  • set home page url with multiple url's value for example https://start.duckduckgo.com/|file:///index.htm|https://google.com/
  • click on new tab
  • only first item should be opened (in this case https://start.duckduckgo.com)

Automated tests

  • npm run watch-test
  • npm run unittest -- --grep="multiple homepages"
  • npm run unittest -- --grep="homepage multiple"

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jan 30, 2017

There is still one open issue with multiple home pages. If you have multiple URL's with a setting A new tab shows set to home page and click on new tab what should happen? Currently it's not working. But if you click on History->Home all URL's are opened. Should we open all URL's when you click on new tab as well?

Settings example (you can also set two websites):
image

@bsclifton @bradleyrichter @bbondy What do you think?

@NejcZdovc NejcZdovc added this to the 0.13.2 milestone Jan 30, 2017
@NejcZdovc
Copy link
Contributor Author

and if we would like to support this officially, then I will add some test's for this features as well

@bradleyrichter
Copy link
Contributor

I think it is reasonable to limit the "new tab" button to the first URL in the series in this case.

@NejcZdovc
Copy link
Contributor Author

Ok will do it like you suggested, thank you

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jan 30, 2017

@bradleyrichter I would also like to add link to the wiki, where user can see how to set up multiple home pages. I created something like this (it's the same as other pages):

screen shot 2017-01-30 at 17 14 20

but I think it would be better if it would be something like this (icon is a link):

image

Which one do you prefer?

@NejcZdovc NejcZdovc force-pushed the hotfix/#6913-multiple-homepages branch from 46f0b12 to 3bbedfb Compare January 30, 2017 16:41
@srirambv
Copy link
Collaborator

Works perfect as per test plan

@NejcZdovc NejcZdovc force-pushed the hotfix/#6913-multiple-homepages branch from 3bbedfb to 790c7bc Compare January 30, 2017 17:53
@bradleyrichter
Copy link
Contributor

@NejcZdovc The "i" button looks like a great solution. Eventually I want to implement a hover balloon for these info buttons that can bring useful quick info vs opening a new wiki tab.

@bsclifton
Copy link
Member

bsclifton commented Jan 30, 2017

Awesome work, @NejcZdovc! 😻

Should we open all URL's when you click on new tab as well?
...
@bsclifton @bradleyrichter @bbondy What do you think?

+1 on what Brad suggested 😄 Just the first one

@bradleyrichter if we wanted to get real fancy, we could create another issue which could introduce a more user-friendly modal. Something like the Windows 10 environment variables editor (only took them 20+ years to add this sucker in)
screen shot 2017-01-30 at 11 10 01 am

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jan 30, 2017

@bsclifton PR is not ready for a review, I still need to add test's, so this can be checked in the future automatically

@bsclifton
Copy link
Member

@bradleyrichter let me know what you think about the implementation 😄 Once it looks good to you, I'll check out the code

@NejcZdovc if you haven't already started, would you mind adding some tests? Thanks!

@bsclifton
Copy link
Member

bsclifton commented Jan 30, 2017

@NejcZdovc you beat me to it!! 😛

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jan 30, 2017

First. I win 🎉 😄

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.

looks good to me!

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Tested it out, works great 😄 Good job with the tests too!

@bsclifton bsclifton modified the milestones: 0.13.2, 0.13.3 Feb 1, 2017
@bsclifton
Copy link
Member

bsclifton commented Feb 1, 2017

Since this is fixing a regression and because it has such greats tests, I think this is a good candidate to pull into 0.13.2 😄 Squashing and merging!

Resolves brave#6913

Auditors:
@bsclifton, @bradleyrichter

Test Plan:
- set home page url with multiple url's value
- home pages should be opened correctly

Includes automated tests

Also see wiki entry at https://github.com/brave/browser-laptop/wiki/End-User-FAQ#how-to-set-up-multiple-home-pages
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.

5 participants