Closed Bug 917822 Opened 10 years ago Closed 9 years ago

[SMS] Use Full Line Highlights

Categories

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

All
Other
defect
Not set
normal

Tracking

(blocking-b2g:-)

RESOLVED WORKSFORME
blocking-b2g -

People

(Reporter: epang, Assigned: arnau)

References

Details

(Whiteboard: visual design, visual-tracking, bokken [mentor=:julienw])

Attachments

(3 files)

Attached image SMS.png
Use full width highlights (edge to edge) for active/pressed states.
Flagging as 1.3? since the highlight state currently looks broken.
blocking-b2g: --- → 1.3?
Whiteboard: visual design, visual-tracking → visual design, visual-tracking, jian
Eric, is the attachment the current behaviour, or the expected behaviour ?

Also, I think it's purely a building block issue.
(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.
triage: would not block release but please land it when its ready
blocking-b2g: 1.3? → -
Whiteboard: visual design, visual-tracking, jian → visual design, visual-tracking, jian [mentor=:julienw]
Pavel, just want to bring this bug to your attention in case if fell off your radar :).  Thanks!
Flags: needinfo?(pivanov)
yep ... I need a device for this one it's hard to simulate the SMS App on the browser
Flags: needinfo?(pivanov)
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
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?
Flags: needinfo?(epang)
(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!
Flags: needinfo?(epang) → needinfo?(pivanov)
Assignee: pivanov → arnau
(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 :)
Flags: needinfo?(pivanov) → needinfo?(arnau)
Whiteboard: visual design, visual-tracking, jian [mentor=:julienw] → visual design, visual-tracking, bokken [mentor=:julienw]
Blocks: SysFE
No longer blocks: 913024
Attached file patch.html
Attachment #8350017 - Flags: review?(borja.bugzilla)
Flags: needinfo?(arnau)
Attached image Thread list in master
When tapping the whole area is highlighted as expected.
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!
Flags: needinfo?(epang)
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!
Attachment #8350017 - Flags: review?(borja.bugzilla)
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.
Flags: needinfo?(epang) → needinfo?(arnau)
WORKSFORME then ? :)
Eric, maybe we need to review Contacts app (FB import) then, to be consistent.
Flags: needinfo?(arnau) → needinfo?(epang)
(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
Flags: needinfo?(epang) → needinfo?(arnau)
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.
Flags: needinfo?(arnau)
Flags: needinfo?(epang)
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!
Flags: needinfo?(epang) → needinfo?(arnau)
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 :)
Flags: needinfo?(arnau)
Blocks: sms-visual-refresh
No longer blocks: SysFE
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.