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.
Created attachment 737626 [details] [review] Fix for 861227
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.
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?
[: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.
Rick, if you're ok with closing this duplicate, I'll let you do that.
ok so we're not closing it after all, but this will need to be rebased after the layout bug lands.
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?
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.
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!
blocks Bug 837994 that is due for leo -> leo?
Created attachment 744105 [details] Pull Request Take a look while fixing the tests!
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.
I'll address things pending tomorrow morning, so It will be ready for reviewing during the evening. Thanks Julien!
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.
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 on attachment 744105 [details] Pull Request Addressed comments given in feedback step. This layout let us create all interactions for multi-recipient.
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.
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.
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.
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 on attachment 744105 [details] Pull Request removing myself as I won't be able to review before next Monday.