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

Status: RESOLVED DUPLICATE of bug 837994

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

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Rick Waldron [:rwaldron]]

Attachment: Fix for 861227

Comment 2

[Alex Keybl [:akeybl]]

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.

Comment 3

[Borja Salguero [:borjasalguero]]

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?

Comment 4

[Julien Wajsberg [:julienw]]

[: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.

Comment 5

[Julien Wajsberg [:julienw]]

Rick, if you're ok with closing this duplicate, I'll let you do that.

Comment 6

[Julien Wajsberg [:julienw]]

ok so we're not closing it after all, but this will need to be rebased after the layout bug lands.

Comment 7

[Borja Salguero [:borjasalguero]]

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?

Comment 8

[Julien Wajsberg [:julienw]]

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.

Comment 9

[Borja Salguero [:borjasalguero]]

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!

Comment 10

[Julien Wajsberg [:julienw]]

blocks Bug 837994 that is due for leo -> leo?

Comment 11

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Take a look while fixing the tests!

Comment 12

[Julien Wajsberg [:julienw]]

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.

Comment 13

[Borja Salguero [:borjasalguero]]

I'll address things pending tomorrow morning, so It will be ready for reviewing during the evening. Thanks Julien!

Comment 14

[Julien Wajsberg [:julienw]]

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.

Comment 15

[Borja Salguero [:borjasalguero]]

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'.

Comment 16

[Borja Salguero [:borjasalguero]]

Comment on attachment 744105 [details]
Pull Request

Addressed comments given in feedback step. This layout let us create all interactions for multi-recipient.

Comment 17

[Julien Wajsberg [:julienw]]

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.

Comment 18

[Borja Salguero [:borjasalguero]]

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.

Comment 19

[Julien Wajsberg [:julienw]]

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.

Comment 20

[Borja Salguero [:borjasalguero]]

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.

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 744105 [details]
Pull Request

removing myself as I won't be able to review before next Monday.