1023819 - [Follow up] Get "Update search app to align with new homescreen" to pass UX review for FL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[Eric Pang [:epang] UX]

This is a follow up to bug 1009401 containing issues with the current implementation of the search app

Visual
1. RB search labels should have 3 lines of text (and larger vertical spacing between icons - extra 20px as seen in spec).

2. Search Result icons are a little small compared to the homescreen (they should be 64x64px)

3. Search Result icons are blurry 

4. Go button is missing in RB

5. Search suggestions colour should be #00aacc (text) and #666666 (bullets).

6. Entering RB Search motion - Based on comment 3 is it the animation that isn’t running good?  Can you let me know, cause if that’s the case we can try something else out (maybe something similar to entering edit mode on homescreen - cause that seems to work really smoothly)

7. some search results icons seem to be missing

Interaction:
1. The spec states that if the user taps the close button, the search field should be cleared and the user should be returned to the home screen. The next time the user accesses the search app, it should be in the empty state (as the search field was cleared). In the build, however, the previous text remains and is shown highlighted. 

2. Conversely, if the user opens the search app and the text field has not been cleared, it should show the previous text AND results. The build, however, only shown the text highlighted, but no results. According to the spec, the text should not be highlighted and the search results should be shown.

I've attached
Please let us know if you have any questions, thanks Dale!

Comment 1

[Eric Pang [:epang] UX]

I've also attached the specs to the user story. THanks!

Comment 2

[Eric Pang [:epang] UX]

Please flag Francis and I for Ui-Review when ready, thanks!

Comment 3

[Jason Smith [:jsmith]]

I'm updating the title here to indicate that this is bug is tracking the need to get the search app to pass UX review.

Comment 4

[Jason Smith [:jsmith]]

Stephany - Can you add the feature-b2g 2.0 flag here? I want to make sure we've got a bug flagged here tracking the need to get UX review to pass post landing.

Comment 5

[Johan Lorenzo [:jlorenzo]]

Marked as 2.0? blocker as it's a feature for 2.0.

Comment 6

[Francis Djabri [:djabber]]

In response to bug #1021418, the Go button has been replaced with a clear button to follow established patterns. This has had some impact on the IxD spec: 

> 
> 4. Go button is missing in RB
> 
This should be replaced with "Clear button is missing in RB"

> 
> Interaction:
> 1. The spec states that if the user taps the close button, the search field
> should be cleared and the user should be returned to the home screen. The
> next time the user accesses the search app, it should be in the empty state
> (as the search field was cleared). In the build, however, the previous text
> remains and is shown highlighted. 

The close button now no longer clears the search field - it just closes the app but maintains its state.

Comment 7

[Eric Pang [:epang] UX]

(In reply to Francis Djabri [:djabber] from comment #6)
> In response to bug #1021418, the Go button has been replaced with a clear
> button to follow established patterns. This has had some impact on the IxD
> spec: 
> 
> > 
> > 4. Go button is missing in RB
> > 
> This should be replaced with "Clear button is missing in RB"
> 
> > 
> > Interaction:
> > 1. The spec states that if the user taps the close button, the search field
> > should be cleared and the user should be returned to the home screen. The
> > next time the user accesses the search app, it should be in the empty state
> > (as the search field was cleared). In the build, however, the previous text
> > remains and is shown highlighted. 
> 
> The close button now no longer clears the search field - it just closes the
> app but maintains its state.

I've update the specs on box, I've also added the clear icon here:
https://mozilla.box.com/s/2h15e0xrtr4uluyj5w2q

Dale, do you think we can add the clear button is this bug?  If so we can probably close bug 1021418.

Comment 8

[Dale Harvey (:daleharvey)]

Ill figure out when implementing them if they are worth being seperate patches or not and if so just close this out

Comment 9

[Dale Harvey (:daleharvey)]

Ok, I will likely split this into a few bugs, implemented the fixes to the input behaviour + persistence and the clear button but not the grid changes

open questions

1. Is there a spec with the go button, clear button and close button, I expect we dont want all 3 shown at the same time?

2. Why does the grid have extra spacing for 3 labels? we use the same rendering code for the homescreen and the search app, the behaviour will be the same

Comment 10

[Eric Pang [:epang] UX]

(In reply to Dale Harvey (:daleharvey) from comment #9)
> Ok, I will likely split this into a few bugs, implemented the fixes to the
> input behaviour + persistence and the clear button but not the grid changes
> 
> open questions
> 
> 1. Is there a spec with the go button, clear button and close button, I
> expect we dont want all 3 shown at the same time?
> 
> 2. Why does the grid have extra spacing for 3 labels? we use the same
> rendering code for the homescreen and the search app, the behaviour will be
> the same

Hi Dale, no prob about splitting into a few bugs - just let us know the bug numbers when ready.

Regarding your questions:

1. The clear button takes the place of the go button.  We're removed the go button - the user can 'go' by using the keyboard.  So only the close and clear buttons are now shown.

2. For the spacing, we wanted to follow what the current version of search is doing.  Many of the e.me search labels tend to be longer so it makes sense to have extra vertical space for them especially for different localizations.  Is this really difficult? 

Thanks! 
Eric

Comment 11

[Dale Harvey (:daleharvey)]

Split into seperate bugs: 

https://bugzilla.mozilla.org/show_bug.cgi?id=1026157 - The open animation was problematic first time I implemented it, it didnt perform great, we cant use css3 transforms since its the height that needs animated

https://bugzilla.mozilla.org/show_bug.cgi?id=1026159 - The grid UX changes, including the 3 lines for labels

The clear button, changes in how the search terms persist when pressing the close button and suggestions colour changes are implemented in this bug

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20584

Implements the clear button, fixes the persistence of the search term and minor visual fixes

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8440948 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20584

Code looks good to me, thanks!

Comment 14

[Francis Djabri [:djabber]]

Thanks Dale. This looks good to me but there's one interaction that still doesn't match the spec - namely, if the user selects one of the e.me search suggestions, the search field should update with the selected term and the keyboard should remain on display. Right now in the latest patch, the keyboard is dismissed if the user taps a search suggestion I've raised Bug #1026716 so we can close this bug off.

Comment 15

[Francis Djabri [:djabber]]

Attachment: Bug 1023819 screens.zip

Comment 16

[Dale Harvey (:daleharvey)]

Thanks Francis, thats a seperate bug so will take that one and landed there changes as is, itd easier to follow up bugs like there

https://github.com/mozilla-b2g/gaia/commit/e4c440ba3996988fb0b749c7034d7990e19601e4

Comment 17

[Eric Pang [:epang] UX]

Comment on attachment 8440948 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20584

Hey Dale,

Most things look good, but there's a few problems I'm still seeing

When I flash with:
DEVICE_DEBUG=1 GAIA_DEV_PIXELS_PER_PX=1.5 make reset-gaia 
I'm seeing a clear button that is scaled too large

See attached screen
http://cl.ly/image/1e3v0r2O1Z1o

Also, is anything we can do with the lower quality icons?  Or is this an search provider problem, if it is and we can't do anything then it's fine.
http://cl.ly/image/24372m2A0y05

Sorry for the delay in review I was having problems flashing.  Since this has already landed will need bugs need to be opened for the icons and potentially lower quality icons? Thanks!

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8440948 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20584

This is needed for the vertical homescreen. We've done our best effort at testing this and believe it is safe for uplift.

Comment 20

[Dale Harvey (:daleharvey)]

Eric, the icons come from the provider, I not sure what we can do about it but definitely doesnt come under this bug, thanks for catching the icon size, I posted a follow up to make sure that works

https://github.com/mozilla-b2g/gaia/commit/8a239771e739f113241f20aa9566387adc824e0d

Comment 21

[Eric Pang [:epang] UX]

(In reply to Dale Harvey (:daleharvey) from comment #20)
> Eric, the icons come from the provider, I not sure what we can do about it
> but definitely doesnt come under this bug, thanks for catching the icon
> size, I posted a follow up to make sure that works
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 8a239771e739f113241f20aa9566387adc824e0d

np, thanks Dale :)

Comment 23

[Kevin Grandon :kgrandon]

2.0: 
https://github.com/mozilla-b2g/gaia/commit/ff88c60655d123b76ddc30894ab94e3126473062
https://github.com/mozilla-b2g/gaia/commit/c45f2a787e81828f8181e897d8e9a4970b2c9581

Comment 24

[Kevin Grandon :kgrandon]

Landed a follow-up to fix the className to follow our platform standards, should've caught it in the review: https://github.com/mozilla-b2g/gaia/commit/e485e031b249bcb135d78fa49b9a4d3ef160e801

Also going to see if we can run this through our CSSLinter.

Comment 25

[Dale Harvey (:daleharvey)]

That last follow up broke the integration tests (and the functionality, yay working integration tests)

filed https://bugzilla.mozilla.org/show_bug.cgi?id=1041319 and landed the fix to get the tree green