917822 - [SMS] Use Full Line Highlights

Status: RESOLVED WORKSFORME

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

Description

[Eric Pang [:epang] UX]

Attachment: SMS.png

Use full width highlights (edge to edge) for active/pressed states.

Comment 1

[Eric Pang [:epang] UX]

Flagging as 1.3? since the highlight state currently looks broken.

Comment 2

[Julien Wajsberg [:julienw]]

Eric, is the attachment the current behaviour, or the expected behaviour ?

Also, I think it's purely a building block issue.

Comment 3

[Eric Pang [:epang] UX]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Eric, is the attachment the current behaviour, or the expected behaviour ?
> 
> Also, I think it's purely a building block issue.

Hi Julien, the attached is the expected behaviors.  Currently the highlight has 15px margins.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

triage: would not block release but please land it when its ready

Comment 5

[Eric Pang [:epang] UX]

Pavel, just want to bring this bug to your attention in case if fell off your radar :).  Thanks!

Comment 6

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

yep ... I need a device for this one it's hard to simulate the SMS App on the browser

Comment 7

[Julien Wajsberg [:julienw]]

Pavel, it's actually easy.

Load it in Firefox with a DEBUG=1 DESKTOP=0 profile.

Steps:

DEBUG=1 DESKTOP=0 PROFILE_FOLDER=./profile-nobrowser make
<path to firefox nightly>/firefox -profile ./profile-nobrowser --no-remote http://sms.gaiamobile.org:8080

Comment 8

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Thread list fixed after landing:
https://bugzilla.mozilla.org/show_bug.cgi?id=917824

Eric, I guess we need to fix the edit mode as well right?

Comment 9

[Eric Pang [:epang] UX]

(In reply to Arnau March from comment #8)
> Thread list fixed after landing:
> https://bugzilla.mozilla.org/show_bug.cgi?id=917824
> 
> Eric, I guess we need to fix the edit mode as well right?

Yes, we should, thanks Arnau!

Comment 10

[Eric Pang [:epang] UX]

(In reply to Eric Pang [:epang] from comment #9)
> (In reply to Arnau March from comment #8)
> > Thread list fixed after landing:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=917824
> > 
> > Eric, I guess we need to fix the edit mode as well right?
> 
> Yes, we should, thanks Arnau!

sorry, this need info should have been for Arnau not Pavel :)

Comment 11

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Attachment: patch.html

Comment 12

[Borja Salguero [:borjasalguero]]

Attachment: Thread list in master

When tapping the whole area is highlighted as expected.

Comment 13

[Borja Salguero [:borjasalguero]]

In master, there is no 'highlight' with 'Edit mode' because this was a requirement (basically because in some apps the behavior differs, and sometimes you can only tap on the 'checkbox', so 'highlight the whole area' has no sense). Eric, do you know if there is any change in the specs? Thanks!

Comment 14

[Borja Salguero [:borjasalguero]]

Comment on attachment 8350017 [details]
patch.html

Arnau, currently in master the behavior is the one expected, that's why Im removing the review flag.
Waiting to Eric for getting more info if we need to update the edit mode, but this should be the same in all apps with the list (so it should be a BB change). Ask me to review again if needed! Thanks!

Comment 15

[Eric Pang [:epang] UX]

Thanks Borja,

Looking at this in more detail I don't think we need a highlight state in the 'edit mode'.  I don't think it's needed since hitting the text does not do anything (and the highlight should signify an action).  When tapped I think it's good enough to have the checkbox turn on and off.  Sorry for the confusion Arnau.

Comment 16

[Julien Wajsberg [:julienw]]

WORKSFORME then ? :)

Comment 17

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Eric, maybe we need to review Contacts app (FB import) then, to be consistent.

Comment 18

[Eric Pang [:epang] UX]

(In reply to Arnau March from comment #17)
> Eric, maybe we need to review Contacts app (FB import) then, to be
> consistent.

Don't think this is worksforme yet :).
From the comment from Borja, we need to remove the highlight from the sms edit screen (the press should just toggle the check box)

Also, I'm having trouble getting FB import to work.  But I believe it should function the same way as the sms edit screen (is this how it currently behaves?).

Thanks!
Eric

Comment 19

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Eric,
when I implemented full highlights in contacts app, I didn't know what was the behavior when we had ckecks, so I also added the effect.
To make FB import work, you need to add a FB Id, but you can see the same result when exporting contacts to SIM od SD card.
If SMS app works for you now, we should close this bug and open another to fix contacts app.

Comment 20

[Eric Pang [:epang] UX]

Hey Arnau,

Tested by exporting contacts to SD card and it looks good (wasn't able to test using FB again since it says invalid app id and I'm not sure where I can update it).

When in the edit list of SMS I'm still seeing the full row highlight instead of just the check.  Can you take a look?  Or maybe I'm testing with an updated patch?  I used the one attached to this bug.  Let me know if i am.  Thanks!

Comment 21

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Eric, we don't need the patch for that bug :) sorry for that.
When I modified Contacts app, by changing the BB, SMS app got updated as well.
You just need to test master.
The patch attached in this bug was only to create the highlight in edit mode, which initially I thought was the right behavior.
So if master works for you, we have to close this bug without applying any patch :)