If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[sms] Create a separate panel for contact live-search.

RESOLVED DUPLICATE of bug 837994

Status

Firefox OS
Gaia::SMS
P1
normal
RESOLVED DUPLICATE of bug 837994
5 years ago
4 years ago

People

(Reporter: julienw, Assigned: borjasalguero)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

210 bytes, text/html
julienw
: feedback+
Details
(Reporter)

Description

5 years ago
STR:
* have some contacts
* open the SMS writer
* write a long SMS, the input text should take all the available space
* search for a contact to send the message

Expected:
* the search results should be visible and the user should be able to select a contact

Actual:
* the search results are displayed behind the input text and therefore are invisible in this situation

The root cause is that the search results are displayed in the same container as where the SMS are displayed. It should be displayed in its own container instead, so that this container could be positioned on top on the input box.
Assignee: nobody → waldron.rick

Updated

5 years ago
blocking-b2g: leo? → tef?
tracking-b2g18: ? → ---
Created attachment 737626 [details] [review]
Fix for 861227
Attachment #737626 - Flags: review?(felash)

Comment 2

5 years ago
We discussed with Telefonica - this does not seem to be a high priority as compared to other blockers. Please nominate for uplift to v1.1 once a fix is available.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
(Assignee)

Updated

5 years ago
Attachment #737626 - Flags: review?(fbsc)
(Assignee)

Comment 3

5 years ago
I was taking a look to the patch and I dont know why to add all these changes only for modifying a CSS style. Furthermore this work it's done as well in the new layout https://bugzilla.mozilla.org/show_bug.cgi?id=860680. So IMHO we should close this one as 'DUPLICATED'. Wdyt?
(Reporter)

Comment 4

5 years ago
[:borjasalguero] from comment #3)
> I was taking a look to the patch and I dont know why to add all these
> changes only for modifying a CSS style.

As explained in the PR, I think you can't only modify the CSS style in this case. I haven't look very deeply into it though.

> Furthermore this work it's done as
> well in the new layout https://bugzilla.mozilla.org/show_bug.cgi?id=860680.
> So IMHO we should close this one as 'DUPLICATED'. Wdyt?

I don't mind doing this work in Bug 860680 except that bug has less priority right now. If you  make it leo+ then ok.
(Reporter)

Comment 5

5 years ago
Rick, if you're ok with closing this duplicate, I'll let you do that.
(Reporter)

Comment 6

5 years ago
ok so we're not closing it after all, but this will need to be rebased after the layout bug lands.
(Assignee)

Comment 7

5 years ago
Rick, I think that we could apply your idea of having a separate panel with the new layout, so you should rebase after landing the new layout. Wdyt?
(Assignee)

Updated

5 years ago
Depends on: 860680
Summary: [sms] the contact search results are invisible when the input text takes all the available space → [sms] Create a separate panel for contact live-search.
(Assignee)

Updated

5 years ago
Blocks: 837994
(Reporter)

Comment 8

5 years ago
Various comments I had on the code that will land in the new layout :

* we need a fixed element for the live search; it will always be in the DOM, since the start
* use event delegation for the list items
* use document fragment when rendering, to avoid passing the container as argument to renderContact (possibly). Or maybe that now that the container is always there, renderContact can maybe directly append to it. Yeah that sounds good.
(Assignee)

Comment 9

5 years ago
Im gonna take this one while Rick is out trying to fix it. Rick, feel free to update the status again (it's only for getting this working asap). THanks!
Assignee: waldron.rick → fbsc
(Reporter)

Updated

5 years ago
Attachment #737626 - Attachment is obsolete: true
Attachment #737626 - Flags: review?(felash)
Attachment #737626 - Flags: review?(fbsc)
(Reporter)

Comment 10

5 years ago
blocks Bug 837994 that is due for leo -> leo?
blocking-b2g: - → leo?
blocking-b2g: leo? → leo+
Priority: -- → P1
(Assignee)

Comment 11

5 years ago
Created attachment 744105 [details]
Pull Request

Take a look while fixing the tests!
Attachment #744105 - Flags: feedback?(felash)
(Reporter)

Comment 12

5 years ago
looks good, but please see my comment 8, as the 3rd suggestion is not done yet. Ie no need to pass the container as an argument to renderContact.

Also, please group your css properties by "type" (ie: layout properties first, style properties then, etc), with an empty line to separate them, so that it's more readable. See for example https://github.com/necolas/idiomatic-css in the part "declaration order". No need to change the styles you don't touch, just do it for the properties you change.
(Reporter)

Updated

5 years ago
Attachment #744105 - Flags: feedback?(felash) → feedback+
(Assignee)

Comment 13

5 years ago
I'll address things pending tomorrow morning, so It will be ready for reviewing during the evening. Thanks Julien!
(Reporter)

Comment 14

5 years ago
Borja, I think Rick worked on the multi recipient patch, and this should probably wait until Rick's patch lands because this introduces some markup change and I don't want that both stuff conflicts.
(Assignee)

Comment 15

5 years ago
Julien, this new markup it's needed for achieving all visual effects that Multirecipient has, due to there  are more actions in this panel (not only tap/click). Check the interaction https://www.dropbox.com/s/fdxgeay9uq9yip1/HTML5_SMS-MMSUserStorySpecifications_20130429_V7.0.pdf, section 'multi-recipients flow'.
(Assignee)

Comment 16

5 years ago
Comment on attachment 744105 [details]
Pull Request

Addressed comments given in feedback step. This layout let us create all interactions for multi-recipient.
Attachment #744105 - Flags: review?(felash)
(Reporter)

Comment 17

5 years ago
I feel like you and rick are doing the same thing in the same time. I don't want to be in the middle of this, please talk together.

I've reviewed the patch, but I think we should wait for Rick's patch that will probably land tonight, because this will probably make this patch obsolete.
(Assignee)

Comment 18

5 years ago
Julien,
Rick's patch it's not adding a separate panel for multirecipient stuff as we were discussing previously (check https://github.com/mozilla-b2g/gaia/pull/9521/files#L0R109). If the separate panel works as expected, should be the pattern followed by Rick's patch (I would be happy if Rick's patch would have this separate panel, but it's not).
We should not block on multiple reviews, because we agree that we need this new panel.
(Reporter)

Comment 19

5 years ago
I haven't blocked, I did your review. I just feel like this will ultimately conflict and therefore we'll _need_ to rework on of the patches anyway. I'd rather that you redo this patch on top of Rick's patch because this patch is smaller and easier than the contrary. Also, the multi recipient patch is imho more important, whereas this patch might wait a few days more. 

I'd expect that you agree with this and we'll land this for sure early next week.
(Assignee)

Updated

4 years ago
Depends on: 865411
(Assignee)

Comment 20

4 years ago
This one have to be landed after https://bugzilla.mozilla.org/show_bug.cgi?id=865411 due to some z-index issues. It's a key point for having all layers working as expected.
(Reporter)

Comment 21

4 years ago
Comment on attachment 744105 [details]
Pull Request

removing myself as I won't be able to review before next Monday.
Attachment #744105 - Flags: review?(felash)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 837994
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.