Closed Bug 1022053 Opened 8 years ago Closed 8 years ago
Add notification in search app for users to configure search provider
No description provided.
Francis, this is the bug tracking the implementation of https://mozilla.app.box.com/s/0i2j11d3odruziews0r7 I realised that we probably need some visuals for having suggestions disabled, as there will be no icon, if we hide the suggestions row / dont display something we will be hiding a way we have taught the user to configure that.
Implements the type notice that tells the user how to configure search providers and the other UX tweaks as specified in https://bugzilla.mozilla.org/show_bug.cgi?id=948320 I have left the 'icon' to be a white circle so it will be easier to see in testing, that will change on the parent bug landing (and based on feedback from francis) Pavel, also not keen on this styling being in here, I was wondering if there was any shared styles I should pull in for this / cleaner implementation I figured the functionality can be reviewed prior to fixing up the icon logic as that will be a small change to follow up.
Comment on attachment 8436256 [details] https://github.com/daleharvey/gaia/commit/de913292fd382ba42ec6d24c24a97d326cfe0c6b I would like to see Pavel or Francis respond first. The white dot looks a bit weird, and this close to FL I think we can't land incomplete things to master. The patch generally looks good, but I'm hesitant to R+ it because I'm not sure about landing order. I'm also not seeing the notification 100% of the time for some reason. Am I supposed to?
I think there is some confusion here. The visual design at https://mozilla.app.box.com/s/0i2j11d3odruziews0r7 is based on the interaction design at https://mozilla.app.box.com/s/5dy7gllr9k3xaro44d07 but that interaction design was the best-case scenario of three different design options (https://mozilla.app.box.com/s/bb95jlbdk9l0robxluxc/1/1955330942) where we support multiple search suggestion providers (a stretch goal for 2.0). 1) Ideal scenario - multiple search suggestions providers, icon in settings app shows the current provider and acts as a shortcut to settings. 2) EverythingMe suggestions - Search suggestions are always provided by EverythingMe, no icon is shown in the search app because it would be confusing (search results come from one provider, suggestions from another). 3) No suggestions - Failing 2, no suggestions should be shown at all if the provider is anything other than EverythingMe. The actual interaction spec that was implemented was 2) https://mozilla.app.box.com/s/bb95jlbdk9l0robxluxc/1/1955330942/17595207603/1 where we don't show the provider icon next to suggestions as a shortcut to settings.
Flagging Eric since he owns VxD here.
The notification is shown one (after typing 3 characters), once its shown once it is not shown again. Based on this mornings meetings, I think we mostly just want an icon that isnt specific to any search provider so the user can still get the notice and navigate to configure their providers
New patch based on updated specs, the notification is only to be shown one time and when dismissed it will not show again.
Running a build @ https://travis-ci.org/daleharvey/gaia/builds/27168179 Seems to be a preexisting intermittent with the vertical homescreen (icon_retriever)
Comment on attachment 8437200 [details] [review] https://github.com/mozilla-b2g/gaia/pull/20248 Looks good! Going to leave some nits on github. Would like to see a rebased and green travis run before giving an R+.
Comment on attachment 8437200 [details] [review] https://github.com/mozilla-b2g/gaia/pull/20248 Patch has been updated with nits addressed
Theres a travis run @ https://travis-ci.org/daleharvey/gaia/builds/27417275 which should kick off shortly
Some genuine failing tests, taking a look
Comment on attachment 8437200 [details] [review] https://github.com/mozilla-b2g/gaia/pull/20248 Sounds good. Flag me again when you think it's ready. Thanks!
Comment on attachment 8437200 [details] [review] https://github.com/mozilla-b2g/gaia/pull/20248 Fixed tests, we now use and confirm the notice during integration tests, so this is very well tested `apps/search/test/marionette/app_search_test.js` has extra code to switch the frame + focus so we can use the same triggerFirstRun whether we are in full rocketbar or hoomescreen. https://travis-ci.org/daleharvey/gaia/builds/27452219 is pretty much a green run, the failing integration test is | 1) Contacts > Delete > Edit menu is not visible on search mode:| which is definitely unrelated
Comment on attachment 8437200 [details] [review] https://github.com/mozilla-b2g/gaia/pull/20248 I left a few comments on github. Assuming you address them, and have a green build somewhere, R+ from me. Thanks a lot!
Attachment #8437200 - Flags: review?(kgrandon) → review+
Candice - Give that this is a feature, can you tag this with feature-b2g?
feature-b2g: --- → 2.0
Addressed the comments, green build @ https://travis-ci.org/daleharvey/gaia/builds/27522253 landed in https://github.com/mozilla-b2g/gaia/commit/18e592f53add20d36e6d40937e2714d8be9d067d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
"Ok" is not correct in English, it should be spelled OK. No need to change the string ID for this kind of change.
Apologies, was as specified, but now you point it out its obviously wrong, minor fix so followed it up https://github.com/mozilla-b2g/gaia/commit/c588bdd1b6e02d294d2b33fe28487e325f2167c9 Note for uplifting, both the following commits need uplifted https://github.com/mozilla-b2g/gaia/commit/18e592f53add20d36e6d40937e2714d8be9d067d https://github.com/mozilla-b2g/gaia/commit/c588bdd1b6e02d294d2b33fe28487e325f2167c9
(In reply to Dale Harvey (:daleharvey) from comment #20) > Note for uplifting, both the following commits need uplifted If this bug is going to be uplifted, it's missing late-l10n in the keywords and approval? flag for 2.0
Comment on attachment 8437200 [details] [review] https://github.com/mozilla-b2g/gaia/pull/20248 NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] [Bug caused by] (feature/regressing bug #): New Feature [User impact] if declined: Inadequate [Testing completed]: Used in hot path of integration tests [Risk to taking this patch] (and alternatives if risky): minor [String changes made]: 2 new strings
Attachment #8437200 - Flags: approval-gaia-v2.0?
Attachment #8437200 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0?(bbajaj)
Obsoleting the pull request in favor of the commit that actually landed to ease uplifting. Carrying 2.0 approval request.
Attachment #8440311 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
On master: * Saw the notification after typing 3 characters. * Notification is still present while keeping to type after it pop'd. * After tapping OK, the notification doesn't show up anymore after typing more character or performing anther search. * Pop-up is not shown anymore after hitting the close or home button. It's OK for me on these points. Still an issue after hitting Enter, the notification is not dismissed as per specs: https://mozilla.app.box.com/s/bb95jlbdk9l0robxluxc/1/1955330942/18050739494/1 Bug 1026007 has been filed.
Status: RESOLVED → VERIFIED
This issue has been verified successfully on Flame 2.0 & 2.1. See attachment: Verify_Video_Flame.MP4 Reproducing rate: 0/5 Flame v2.0 version: Gaia-Rev 8d1e868864c8a8f1e037685f0656d1da70d08c06 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3 Build-ID 20141202000201 Version 32.0 Flame v2.1 version: Gaia-Rev ccb49abe412c978a4045f0c75abff534372716c4 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22 Build-ID 20141202001201 Version 34.0
You need to log in before you can comment on or make changes to this bug.