Closed Bug 1034600 Opened 6 years ago Closed 6 years ago

[Messages] Suggestions list is badly located

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: julienw, Assigned: azasypkin)

References

Details

(Keywords: regression, Whiteboard: [p=2])

Attachments

(3 files)

Attached image 2014-07-04-15-30-48.png
Hey Victoria,

I think the suggestion list is badly placed now: there is a huge padding between the list and the recipients panel.

Can you confirm it's wrong?
Flags: needinfo?(vpg)
Yes, of course it is wrong.
Flags: needinfo?(vpg)
I think this is a regression from bug 949457, but the underlying reason is that we use the same element to show the messages in a thread and to show the contact suggestion list. This is really wrong and in this bug we need to change it too.
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S6 (18july)
blocking-b2g: 2.0? → 2.0+
Keywords: regression
Status: NEW → ASSIGNED
OS: Linux → Gonk (Firefox OS)
QA Contact: azasypkin
Hardware: x86_64 → ARM
Assignee: nobody → azasypkin
QA Contact: azasypkin
Hey Julien,

Here is the patch I came up with, could you please review it?

Thanks!
Attachment #8454303 - Flags: review?(felash)
Comment on attachment 8454303 [details] [review]
GitHub pull request URL

Looks good but I added some comments on the PR. Especially vh is slow (bug 1020469); it's ok to use it if the selector is not used in a place where the keyboard can be displayed/hidden but otherwise we should unfortunately avoid it.

I also think this is a good time to get rid of the dynamic <ul> (maybe even simply replace your new <div> with that <ul>) used for the suggestions list.

Good work, let's do another round !
Attachment #8454303 - Flags: review?(felash)
Comment on attachment 8454303 [details] [review]
GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] (PTO Monday 14th July) from comment #4)
> Comment on attachment 8454303 [details] [review]
> GitHub pull request URL
> 
> Looks good but I added some comments on the PR. Especially vh is slow (bug
> 1020469); it's ok to use it if the selector is not used in a place where the
> keyboard can be displayed/hidden but otherwise we should unfortunately avoid
> it.
> 
> I also think this is a good time to get rid of the dynamic <ul> (maybe even
> simply replace your new <div> with that <ul>) used for the suggestions list.
> 
> Good work, let's do another round !

Hey Julien,

Please see updated PR (in a separate commit).

Thanks!
Attachment #8454303 - Flags: review?(felash)
Comment on attachment 8454303 [details] [review]
GitHub pull request URL

I added some more comments; especially I'd rather set up the events once for all at init; and I think we should show/hide the element explicitely instead of relying to its height being 0.
Attachment #8454303 - Flags: review?(felash)
Comment on attachment 8454303 [details] [review]
GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] (PTO Monday 14th July) from comment #6)
> Comment on attachment 8454303 [details] [review]
> GitHub pull request URL
> 
> I added some more comments; especially I'd rather set up the events once for
> all at init; and I think we should show/hide the element explicitely instead
> of relying to its height being 0.

Fixed nits and added show\hide behaviour.

Thanks!
Attachment #8454303 - Flags: review?(felash)
Comment on attachment 8454303 [details] [review]
GitHub pull request URL

r=me with some non-blocking nits, so it's up to you.

thanks !
Attachment #8454303 - Flags: review?(felash) → review+
(In reply to Julien Wajsberg [:julienw] from comment #8)
> Comment on attachment 8454303 [details] [review]
> GitHub pull request URL
> 
> r=me with some non-blocking nits, so it's up to you.
> 
> thanks !

Thanks for review!

Master: https://github.com/mozilla-b2g/gaia/commit/0a0b4c9d4bccb4fb96fba1e8b855b1afdc96fb31
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1047475
Hi Julien, 
Could you please provide a repro steps, thanks!
Flags: needinfo?(felash)
STR:
0. Prerequisite: have several contacts starting with "A"
1. Open Messages app
2. press "new message" button
3. tap the recipient panel to be able to enter new recipients
4. enter "A"

=> The suggestions list panel should start right below the recipient panel.
Flags: needinfo?(felash)
Attached video video
According to the STR in Comment#12, this issue has been verified successfully on Flame 2.0 and 2.1
See attachment: Verify_1034600.MP4
Reproducing rate: 0/5

Flame 2.0 build:
Gaia-Rev        856863962362030174bae4e03d59c3ebbc182473
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1
Build-ID        20141208000206
Version         32.0

Flame 2.1 build:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.