Closed
Bug 861227
Opened 11 years ago
Closed 11 years ago
[sms] Create a separate panel for contact live-search.
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect, P1)
Tracking
(blocking-b2g:leo+, b2g18+)
RESOLVED
DUPLICATE
of bug 837994
blocking-b2g | leo+ |
Tracking | Status | |
---|---|---|
b2g18 | + | --- |
People
(Reporter: julienw, Assigned: borjasalguero)
References
Details
Attachments
(1 file, 1 obsolete file)
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.
Updated•11 years ago
|
Assignee: nobody → waldron.rick
Updated•11 years ago
|
blocking-b2g: leo? → tef?
tracking-b2g18:
? → ---
Comment 1•11 years ago
|
||
Attachment #737626 -
Flags: review?(felash)
Comment 2•11 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•11 years ago
|
Attachment #737626 -
Flags: review?(fbsc)
Assignee | ||
Comment 3•11 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•11 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•11 years ago
|
||
Rick, if you're ok with closing this duplicate, I'll let you do that.
Reporter | ||
Comment 6•11 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•11 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•11 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.
Reporter | ||
Comment 8•11 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•11 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•11 years ago
|
Attachment #737626 -
Attachment is obsolete: true
Attachment #737626 -
Flags: review?(felash)
Attachment #737626 -
Flags: review?(fbsc)
Reporter | ||
Comment 10•11 years ago
|
||
blocks Bug 837994 that is due for leo -> leo?
blocking-b2g: - → leo?
Updated•11 years ago
|
blocking-b2g: leo? → leo+
Priority: -- → P1
Assignee | ||
Comment 11•11 years ago
|
||
Take a look while fixing the tests!
Attachment #744105 -
Flags: feedback?(felash)
Reporter | ||
Comment 12•11 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•11 years ago
|
Attachment #744105 -
Flags: feedback?(felash) → feedback+
Assignee | ||
Comment 13•11 years ago
|
||
I'll address things pending tomorrow morning, so It will be ready for reviewing during the evening. Thanks Julien!
Reporter | ||
Comment 14•11 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•11 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•11 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•11 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•11 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•11 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 | ||
Comment 20•11 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•11 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•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Updated•11 years ago
|
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.
Description
•