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

RESOLVED FIXED in Firefox OS v2.0

Status

Firefox OS
Gaia::Search
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: epang, Assigned: daleharvey)

Tracking

unspecified
2.0 S4 (20june)
x86
Gonk (Firefox OS)
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(feature-b2g:2.0, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [systemsfe])

User Story

Interaction Spec:
https://mozilla.app.box.com/files/0/f/1955330942

Visual Specs
https://mozilla.box.com/s/ho1brfbe9f4sw5uv3ga4
Image assets (clear icon):
https://mozilla.box.com/s/2h15e0xrtr4uluyj5w2q

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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!
(Reporter)

Comment 1

4 years ago
I've also attached the specs to the user story. THanks!
(Reporter)

Comment 2

4 years ago
Please flag Francis and I for Ui-Review when ready, thanks!
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.
Summary: [Follow up] Update search app to align with new homescreen → [Follow up] Get "Update search app to align with new homescreen" to pass UX review for FL
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.
Flags: needinfo?(swilkes)

Updated

4 years ago
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+]

Updated

4 years ago
feature-b2g: --- → 2.0
Flags: needinfo?(swilkes)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S4 (20june)
blocking-b2g: --- → 2.0?
Marked as 2.0? blocker as it's a feature for 2.0.
blocking-b2g: 2.0? → ---
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.
(Reporter)

Comment 7

4 years ago
(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.
User Story: (updated)
Flags: needinfo?(dale)
(Assignee)

Comment 8

4 years ago
Ill figure out when implementing them if they are worth being seperate patches or not and if so just close this out
Flags: needinfo?(dale)
(Assignee)

Comment 9

4 years ago
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
Flags: needinfo?(epang)
(Reporter)

Comment 10

4 years ago
(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
Flags: needinfo?(epang) → needinfo?(dale)
(Assignee)

Comment 11

4 years ago
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
Flags: needinfo?(dale)
(Assignee)

Comment 12

4 years ago
Created attachment 8440948 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20584

Implements the clear button, fixes the persistence of the search term and minor visual fixes
Attachment #8440948 - Flags: ui-review?(epang)
Attachment #8440948 - Flags: review?(kgrandon)
Attachment #8440948 - Flags: feedback?(fdjabri)
Comment on attachment 8440948 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20584

Code looks good to me, thanks!
Attachment #8440948 - Flags: review?(kgrandon) → review+
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.
Attachment #8440948 - Flags: feedback?(fdjabri) → feedback+
Created attachment 8441656 [details]
Bug 1023819 screens.zip
(Assignee)

Comment 16

4 years ago
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

4 years ago
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!
Attachment #8440948 - Flags: ui-review?(epang) → ui-review-
Flags: needinfo?(dale)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1027154
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.
Attachment #8440948 - Flags: approval-gaia-v2.0?(bbajaj)
(Assignee)

Comment 20

4 years ago
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
Flags: needinfo?(dale)
(Reporter)

Comment 21

4 years ago
(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 :)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1021418

Updated

4 years ago
Attachment #8440948 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
2.0: 
https://github.com/mozilla-b2g/gaia/commit/ff88c60655d123b76ddc30894ab94e3126473062
https://github.com/mozilla-b2g/gaia/commit/c45f2a787e81828f8181e897d8e9a4970b2c9581
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Flags: in-moztrap-
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.
(Assignee)

Comment 25

3 years ago
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
You need to log in before you can comment on or make changes to this bug.