Closed Bug 882154 Opened 11 years ago Closed 11 years ago

[MMS] [UX] Multi Recipients. Apply overlay design for contact information

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:-)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g -

People

(Reporter: vicky, Assigned: gduan)

References

Details

(Whiteboard: [leo-triage] )

Attachments

(8 files, 4 obsolete files)

Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411
358 bytes, text/html
borjasalguero
: feedback+
Details
specs for correct layout
111.23 KB, image/png
Details
Spacing and font updated
141.02 KB, image/png
Details
text alignment wrong
62.98 KB, image/png
Details
alignment updated
71.56 KB, image/png
Details
Single recipient header Error
71.89 KB, image/png
Details
Multirecipient is working fine
73.35 KB, image/png
Details
CSS issue. Scroll.
71.83 KB, image/png
Details 
Adjust overlays for contact details/actions in multirecipient. The overlay should use building blocks but the contact information on the top should be displayed as suggested here:

https://www.dropbox.com/sh/7ky6j0wn81l7cl5/q8yfFz3BEY  (04.overlay... files)

Here the specs: https://www.dropbox.com/sh/pa0cqfkdkdybrfb/ikmucnAeZe

Thanks
Blocks: 872514
blocking-b2g: --- → leo?
blocking-b2g: leo? → leo+
Assignee: nobody → mshiao
Assignee: mshiao → gduan
I'd like to confirm two things with you.

1. Only contact information on the top should be modified ?
2. Should all action items look like this cross ? (padding width and the bottom line)
Flags: needinfo?(vpg)
Hi George,
PLeas use everything else as it is in the Building blocks, we do not want inconsistencies. The only thing to modify here is the top of it where the contact name and/or number are displayed. So to show that it needs to be a bit bigger when showing the name.

Thanks!
Flags: needinfo?(vpg)
Comment on attachment 763397 [details]
Pointer to https://github.com/mozilla-b2g/gaia/pull/10411

<!DOCTYPE html><meta charset="utf-8"><meta http-equiv="refresh" content="5;https://github.com/mozilla-b2g/gaia/pull/10293"><title>Bugzilla Code Review</title><p>You can review this patch at <a href="https://github.com/mozilla-b2g/gaia/pull/10293">https://github.com/mozilla-b2g/gaia/pull/10293</a>, or wait 5 seconds to be redirected there automatically.</p>
Comment on attachment 763397 [details]
Pointer to https://github.com/mozilla-b2g/gaia/pull/10411

<!DOCTYPE html><meta charset="utf-8"><meta http-equiv="refresh" content="5;https://github.com/mozilla-b2g/gaia/pull/10411"><title>Bugzilla Code Review</title><p>You can review this patch at <a href="https://github.com/mozilla-b2g/gaia/pull/10411">https://github.com/mozilla-b2g/gaia/pull/10411</a>, or wait 5 seconds to be redirected there automatically.</p>
Comment on attachment 763397 [details]
Pointer to https://github.com/mozilla-b2g/gaia/pull/10411

<!DOCTYPE html><meta charset="utf-8"><meta http-equiv="refresh" content="5;https://github.com/mozilla-b2g/gaia/pull/10411"><title>Bugzilla Code Review</title><p>You can review this patch at <a href="https://github.com/mozilla-b2g/gaia/pull/10411">https://github.com/mozilla-b2g/gaia/pull/10411</a>, or wait 5 seconds to be redirected there automatically.</p>
Attachment #763397 - Attachment mime type: text/plain → text/html
Comment on attachment 763401 [details]
Pointer to https://github.com/mozilla-b2g/gaia/pull/10411

><!DOCTYPE html><meta charset="utf-8"><meta http-equiv="refresh" content="5;https://github.com/mozilla-b2g/gaia/pull/10411"><title>Bugzilla Code Review</title><p>You can review this patch at <a href="https://github.com/mozilla-b2g/gaia/pull/10411">https://github.com/mozilla-b2g/gaia/pull/10411</a>, or wait 5 seconds to be redirected there automatically.</p>
Attachment #763401 - Attachment is obsolete: true
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

Im gonna take a look to your patch. Take into account that reviewer & developer of the patch should not be the same person. Thanks!
Attachment #763407 - Flags: review+ → review?(fbsc)
The file Victoria mentioned before is:
https://www.dropbox.com/sh/7ky6j0wn81l7cl5/q8yfFz3BEY#f:04.SMS_overlay.png

I will take a look to your patch asap!
Please rebase your patch before reviewing it. Thanks!
Borja, Victoria: That file illustrates a deviation from the spec, please see all of the screen shots posted in the PR: https://github.com/mozilla-b2g/gaia/pull/10411#issuecomment-19553756
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

Adding Corey as well!
Attachment #763407 - Flags: review?(gnarf37)
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

Looks good to me, but there is a decent change in CSS - I'd actually like victoria to review this, and also rick, what was the other menu that might be affected by the CSS you mentioned in the pull?
Attachment #763407 - Flags: review?(gnarf37) → review?(vpg)
Hey Corey,
Since it's layout what I am reviewing, can you place a screenshot?

Thanks!

 (In reply to Corey Frang [:gnarf] [:gnarf37] from comment #15)
> Comment on attachment 763407 [details]
> Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411
> 
> Looks good to me, but there is a decent change in CSS - I'd actually like
> victoria to review this, and also rick, what was the other menu that might
> be affected by the CSS you mentioned in the pull?
Attached image overlay design for contact information (obsolete) — 
Here's the screenshot for layout of contact information.
Flags: needinfo?(vpg)
(In reply to George Duan [:gduan] from comment #17)
> Created attachment 766377 [details]
> overlay design for contact information
> 
> Here's the screenshot for layout of contact information.

Hi George,

This design is aproximate btu it's not following the specs, please grab sizes and type style + spacing from the spec I am attaching.
Flags: needinfo?(vpg)
(In reply to George Duan [:gduan] from comment #17)
> Created attachment 766377 [details]
> overlay design for contact information
> 
> Here's the screenshot for layout of contact information.

Hi George,

This design is aproximate btu it's not following the specs, please grab sizes and type style + spacing from the spec I am attaching.
Hi Victoria, 

Are you talking about the difference of font style? I think the width of spacing should be correct.
Flags: needinfo?(vpg)
Hey George, 
I understand that you taking the screen captures from a different source and the font Feura Sans is not that one.

But as far as I can see:
1. The inline spacing between the two lines in the top is bigger in your screen, it should be 2.5 rem line height
2. The text should be Feura Sans Light in both lines.
3. The horizontal divider line in the top should be 0.5 rem higher.

I am attaching the specs file with the screen capture you provided to better show the measures mistakes I mention.

PLease, tell me if there's something else that is not clear for you.

Thanks!
Flags: needinfo?(vpg)
Attached image Spacing issue (obsolete) — 

    
    

    
  


  
The font naturally have some top spacing , so there might be a little different.
I also set the font-family as 'Feura Sans Light' and set font-weight as lighter.

        Attachment #766377 -
        Attachment is obsolete: true
Flags: needinfo?(vpg)

    
    

    
  
Hi George,
Thanks for adjusting this. Looks much better. Only thing I see is that the beginning of the top text is not well aligned, it should line up with the text inside the buttons. Attaching image.
Flags: needinfo?(vpg)

    
    

    
  

      
      
      
      

        Attached image
          
        text alignment wrong
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached image
          
        alignment updated
        — 
      

    


  
update alignment

        Attachment #767721 -
        Attachment is obsolete: true
Flags: needinfo?(vpg)

    
    

    
  
Looks good George, thanks for making this adjustments!
Flags: needinfo?(vpg)

    
    

    
  
Hi Borja ,

Victoria is ok with the UI now . I just rebase on master and fix some conflict , could you please kindly review it again? Thanks
Flags: needinfo?(fbsc)
Whiteboard: [u=commsapps-user c=messaging p=0]

    
    

    
  
Hi George,
Due to the issue of the 'separator' in the carrier stuff now is being handled in other bug, and there are a lot of discussions related with the format, I would like to remove all the stuff related with https://github.com/mozilla-b2g/gaia/pull/10411/files#L2R435 in this PR, and we are going to focus only in the CSS/HTML request by Victoria. Please update the PR with this and I ask me again for the review. Thanks!
Flags: needinfo?(fbsc)

        Attachment #763407 -
        Flags: review?(vpg)

        Attachment #763407 -
        Flags: review?(fbsc)

    
    

    
  
Hi Borja,

I remove everything about separator and re-commit. Please kindly check. Thanks.
Flags: needinfo?(fbsc)
Flags: needinfo?(fbsc)
Whiteboard: [u=commsapps-user c=messaging p=0] → [u=commsapps-user c=messaging p=1]
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM

    
    

    
  


  
This is the issue to fix. Tapping on the header of a 'single recipient' conversation, the header is not the right one.

    
    

    
  


  
However, in the case of tapping on a contact who belongs to a multirecipient conversation, the info is displayed properly.

    
    

    
  
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

Hi George,
Thanks for addressing the change quickly :). I've been testing your patch and I've discovered a weird behaviour of this panel depending on the previous conversation where you are. In the case of 'single' recipient conversation/thread is not working as expected.
Im afraid that we are not filling the info needed in the prompt (probably because the info retrieved from contacts is not in the header), but storing a contact called 'Alberto, 123456789' is shown in the header, but not in the prompt.

Please fix this issue and ask me again for reviewing it. I'll do it for sure asap! Thanks. Gracias!

        Attachment #763407 -
        Flags: review?(fbsc)

    
    

    
  
In reply to comment 34,

Thanks for the test I miss.
Just like you said, the bug is caused by empty name and phone type. 
I revised and updated, please kindly check again. Thanks.
Flags: needinfo?(fbsc)

    
    

    
  
George, I've tested and it's not working ... probably you forgot to push new changes properly?
Flags: needinfo?(fbsc) → needinfo?(gduan)

    
    

    
  
New patch should work like below.

(single receiver)
https://www.dropbox.com/s/7aejex1sxq9zjbb/IMAG1184.jpg
(multi receivers with mms)
https://www.dropbox.com/s/2w6ex28eud7jknp/IMAG1185.jpg
Flags: needinfo?(gduan) → needinfo?(fbsc)

    
    

    
  
> (single receiver)
> https://www.dropbox.com/s/7aejex1sxq9zjbb/IMAG1184.jpg

I think this is wrong... Actually is the same as https://bug882154.bugzilla.mozilla.org/attachment.cgi?id=772566 without one empty line.

In the case of an UNKNOWN phonenumber the UI should be:
https://mozilla.app.box.com/s/wzgsb3lkqglv0dnfdgzs/1/991301651/8884794153/1

In the case of a CONTACT, UI should be:
https://mozilla.app.box.com/s/wzgsb3lkqglv0dnfdgzs/1/991301651/8884792721/1

So you have to fix the following issue. Follow STR:
- Create a contact with name 'Manolo', phone number '12341234'
- Send a SMS for creating a thread
- Tap on the header

EXPECTED:
Something like https://mozilla.app.box.com/s/wzgsb3lkqglv0dnfdgzs/1/991301651/8884792721/1

CURRENTLY:
https://www.dropbox.com/s/7aejex1sxq9zjbb/IMAG1184.jpg

Please fix this issue and let me know for reviewing the code again. Thanks!
Flags: needinfo?(fbsc) → needinfo?(gduan)

    
    

    
  
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

Check latest comment. Thanks!

        Attachment #763407 -
        Flags: review-

    
    

    
  
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

New patch fix below pbs.
1. unknown number, text align to left
2. header of prompt for single sms/mms contact
3. header of prompt for known number but no contact name

        Attachment #763407 -
        Flags: review- → review?(fbsc)

    
    

    
  

      
      
      
      

        Attached image
          
        CSS issue. Scroll.
        — 
      

    


  
Here is an issue in the header. There is an overflow due to the new CSS, so this area is scrollable.

    
    

    
  
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

George, we are quite close for landing this! I need a change in the CSS. With your patch the header show a 'scroll', so we need to fix this! Take a look and let me know when this is ready. It will be merged soon for sure! Thanks a lot!

        Attachment #763407 -
        Flags: review?(fbsc)

        Attachment #763407 -
        Flags: review-

        Attachment #763407 -
        Flags: feedback+

    
  
blocking-b2g: leo+ → leo?
Priority: -- → P2
Whiteboard: [u=commsapps-user c=messaging p=1] → [u=commsapps-user c=messaging p=1],[leo-triage]

    
    

    
  
Comment on attachment 763407 [details]
Pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/10411

Fixed. Thanks.

        Attachment #763407 -
        Flags: review- → review?(fbsc)

    
    

    
  
Not a blocker for partners, and not a blocker for us. Let's fix for 1.2. We don't take non-blocking UX bugs in the last month of the release.
blocking-b2g: leo? → -
Whiteboard: [u=commsapps-user c=messaging p=1],[leo-triage] → [leo-triage]
Flags: needinfo?(gduan)
Blocks: 896368

    
    

    
  
Hi all, 

I'm currently dealing with bug 896368 which tries to show the contact image on the overlay, but guess that any change that I do on there will clash with the changes that this bug will insert, apart from the fact that we currently have no visuals for the use case. 
So, question for Victoria, what would be better?
1. Add the image to the proposition to follow the specs in this same bug
2. Keep this bug as it is and deal with the image only on bug 896368
3. Create a new bug for new changes on the layout to follow specs, and make 896368 depend on it
Flags: needinfo?(vpg)

    
    

    
  
George, we would need to rebase this, could you take a look? Thanks!
Flags: needinfo?(gduan)

    
    

    
  
Hi Borja,

I just rebased, but it seems sth wrong with CI server.
Flags: needinfo?(gduan)

    
    

    
  
In bug 896368 we will apply the changes for showing the contact image on the overlay if needed. Waiting UX input there. By now merging this!

    
    

    
  
Borja, this patch is full of serious bugs. 

- Instead of creating an actual boolean for isContact (from dataset.isContact) it uses the string value, which of course will evaluate to truthy every time. 
  - https://github.com/mozilla-b2g/gaia/commit/371da93dcd39ed5f3950fed96600762c14c80d79#commitcomment-3821205
  - https://github.com/mozilla-b2g/gaia/commit/371da93dcd39ed5f3950fed96600762c14c80d79#commitcomment-3821230

- The tests were rigged/modified so that the broken code would pass
  - https://github.com/mozilla-b2g/gaia/commit/371da93dcd39ed5f3950fed96600762c14c80d79#L3R2099

- New (broken) API surface was added and no new tests were written.

    
    

    
  
Can we revert this commit in master and fix the issues Rick mentioned with it before landing it again?
Flags: needinfo?(gduan)

    
    

    
  
(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #51)
> Can we revert this commit in master and fix the issues Rick mentioned with
> it before landing it again?

I was able to address these issues as part of the fixes included in bug 903137

    
    

    
  
Thanks to address these issues, Rick.
Flags: needinfo?(vpg)
Flags: needinfo?(gduan)

        Attachment #763407 -
        Flags: review?(fbsc)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: