941292 - [l10n] Change - Add 'search settings' to the 'settings' menu

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Karen Rudnitski [:kar]]

User should be able to set / choose their default search provider.

It would be expected that this option is served in Metro Settings.

Yuan added to provide a design that fits into the current settings template to provide this choice (envisioning a list of available search providers and being able to select which becomes your default). Use styling that befits overall design principles :)

Comment 1

[Karen Rudnitski [:kar]]

Clearing 'need info' tags for Yuan and Madhava as Michael is taking lead right now.

Comment 2

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Windows8-i03-SearchDefault_1-2.png

User Flow Part 1: User selects "Search" from the settings menu.

Comment 3

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Windows8-i03-SearchDefault_2-2.png

User Flow Part 2: User selects their default search provider.

Comment 4

[Jim Mathies [:jimm]]

Attachment: re-org settings charm code

Comment 5

[Jim Mathies [:jimm]]

Attachment: add search options v.1

Comment 6

[Jim Mathies [:jimm]]

(In reply to mmaslaney from comment #3)
> Created attachment 8340876 [details]
> Windows8-i03-SearchDefault_2-2.png
> 
> User Flow Part 2: User selects their default search provider.

Note I used radio buttons vs. checkboxes here, since radios imply single selection.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8341727 [details] [diff] [review]
re-org settings charm code

Review of attachment 8341727 [details] [diff] [review]:
-----------------------------------------------------------------

Tidy

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8341728 [details] [diff] [review]
add search options v.1

Review of attachment 8341728 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works well

::: browser/metro/base/content/flyoutpanels/SearchFlyoutPanel.js
@@ +6,5 @@
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; 

nit: trailing whitespace

@@ +19,5 @@
> +    }
> +
> +    Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +    this._isInitialized = true;
> +    let self = this;

nit: if you use an arrow function in the forEach below, you can use 'this' as-is.

@@ +51,5 @@
> +    this._engines = Services.search.getVisibleEngines();
> +    let defaultEngine = Services.search.defaultEngine;
> +
> +    let index = 0;
> +    this._engines.forEach(function (anEngine) {

nit: The 2nd param to your function is an index, no need to declare and manually increment 'index' here

Comment 9

[Jim Mathies [:jimm]]

> nit: if you use an arrow function in the forEach below, you can use 'this'
> as-is.

I was going to do this but realized we didn't need the _elements variable. So I just clipped all this code out.

Comment 10

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/bf8935bf5847
remote:   https://hg.mozilla.org/integration/fx-team/rev/d8c647479ec1

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/bf8935bf5847
https://hg.mozilla.org/mozilla-central/rev/d8c647479ec1