873028 - [MMS] Compose and Reply a message. Implement MMS indicator

Status: RESOLVED FIXED

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

Description

[Victoria Gerchinhoren [:vicky]]

Attachment: shows look and placement of the MMS indicator in the input field

Implement MMS indicator in messages list as shown in the attachment. The size and color of the font is specified there as well.

Comment 1

[Corey Frang [:gnarf]]

Fixed a bug whic

Comment 2

[Corey Frang [:gnarf]]

Attachment: patch v1

Pinging two reviewers, whoever gets here first, please remove the other flag.

This fixed a bug where an image being attached to a message wouldn't cause the MMS indicator to show up.

Also, applies styles to look like the visual mock

Comment 3

[Corey Frang [:gnarf]]

Attachment: Screenshot with patch

Comment 4

[Corey Frang [:gnarf]]

Attachment: Screenshot with patch

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 753397 [details] [diff] [review]
patch v1

Review of attachment 753397 [details] [diff] [review]:
-----------------------------------------------------------------

I see the following problem:
* the counter is not properly placed when it's displayed very soon (eg: try to type a SMS with a character "ê")

see screenshot (don't look at the overing problems due to the fact that I have a big send button, this is bug 871986)

::: apps/sms/style/sms.css
@@ +559,5 @@
>    padding-right: 0.5rem;
>  }
>  
>  [data-message-type="mms"] #messages-send-button.has-counter:before {
>    content: 'MMS';

don't we need a display:block ? otherwise I suspect the padding-left won't apply ?
(but I'm not so sure)

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: screenshot with the counter problem

Comment 7

[Corey Frang [:gnarf]]

So this bug with the counter showing up early was one I discussed with borja at the work week.  The fact that the counter might show up before the input was tall enough to hold it was not considered possible. Darn long chars. 

Borja/Julien - Can you look at the CSS with this in mind and suggest the proper code to use, or can someone else take my patch and fix the CSS correctly? I had another positioning model setup at one point that dealt with this scenario, but it wasn't considered possible :)

Comment 8

[Julien Wajsberg [:julienw]]

as discussed on IRC, will try to find the right CSS tomorrow.

Comment 9

[Julien Wajsberg [:julienw]]

So, I checked 1.0.1, and I really like how the SMS counter is just above the send button.

Victoria, could it be possible to have the SMS character counter like in 1.0.1, which would prevent having the problem I highlight in comment 6 ? We still can put the MMS indicator where you want it to be.

To reproduce easily the comment 6 problem, start a SMS with "Ê" and then add lots of short characters like "l" or "i", the counter will appear before the end of the second line.

Comment 10

[Julien Wajsberg [:julienw]]

(Gave it back to :gnarf because if this simpler solution is agreed by Victoria, he'll be able to move forward)

Comment 11

[Borja Salguero [:borjasalguero]]

Attachment: No issue with height in English

Comment 12

[Borja Salguero [:borjasalguero]]

Attachment: No issue with height in French

Comment 13

[Borja Salguero [:borjasalguero]]

I've seen that there is no issue of the height neither master or v1-train. Corey, this patch is ready for review?

Comment 14

[Borja Salguero [:borjasalguero]]

I've just checked the issue with characters as 'ë' etc... Waiting Vicky!

Comment 15

[Victoria Gerchinhoren [:vicky]]

Hi, so if there's really a problem, please use this layout I am attaching, is the MMS indicator without the vertical line in the left and separated from the send button by a 0.6 rem vertical space. Please make sure you update the counter like this also.

Comment 16

[Victoria Gerchinhoren [:vicky]]

Attachment: illustrates solution for height problems in the counter

Comment 17

[Corey Frang [:gnarf]]

Attachment: screenshot of error with new design spec

This has the changes from victoria, but still causes a problem

Comment 18

[Victoria Gerchinhoren [:vicky]]

I see Corey, thanks for the screenshot. This is no longer a strict visual matter, so pingin Ayman to see if he can give some input here.

Comment 19

[Corey Frang [:gnarf]]

Attachment: patch v2

sets a bunch of absolute positioning to put the counter right above the send button

Comment 20

[Corey Frang [:gnarf]]

Attachment: screenshot with patch - mms

latest patch

Comment 21

[Corey Frang [:gnarf]]

Attachment: Screenshot with patch - short text

Comment 22

[Victoria Gerchinhoren [:vicky]]

Corey, those screenshots are not a valid solution. Will wait for Ayman's input to see if there's a different placement solution for it.

Comment 23

[Corey Frang [:gnarf]]

Comment on attachment 755989 [details]
No issue with height in French

obsoleted borja's screenshots since he wasn't using a ë type character to make the segment length shorter.

Comment 24

[Corey Frang [:gnarf]]

kicking the bug over to borja - I think borja can take my patch and fix the CSS with input from victoria - the code shouldn't need to change at all.

Comment 25

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 26

[Borja Salguero [:borjasalguero]]

Attachment: Patch applied with right aligment

Comment 27

[Borja Salguero [:borjasalguero]]

I've created this path for fixing the functionality (now it's broken), and it's agreed with UX. Furthermore we are going to add the right styles here https://bugzilla.mozilla.org/show_bug.cgi?id=878758 as a followup.

Comment 28

[Corey Frang [:gnarf]]

I'm a little confused here --- This doesn't include the code/unit test fixes from my patch?  Why do we need the followup?  Sounds like your pull is the followup and my old patch is this bug?  I was assuming you would just take my patch and fix the CSS on top of it?

Comment 29

[Borja Salguero [:borjasalguero]]

Hi Corey,
Feel free to take this code and use this CSS in your patch. After talking with UX we are going to fix the issue (because now it's broken), and we are going to work in the followup with UX for creating a better solution aligned with all the requirements (that's why the followup).

Comment 30

[Corey Frang [:gnarf]]

Okay! Thanks - I'm going to take my code, + your css, and r=you and just land it then?

Comment 31

[Borja Salguero [:borjasalguero]]

Merge both codes and I will simply test it and r ok? Thanks!

Comment 32

[Corey Frang [:gnarf]]

Attachment: screenshot with patch - mms

Comment 33

[Corey Frang [:gnarf]]

Attachment: Screenshot with patch - short text

Comment 34

[Corey Frang [:gnarf]]

Attachment: patch v3

Here is an updated patch Borja - Please check it out

Comment 35

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/64ab5d6649a3227a98c5808d3eaa8910d571d3a1

Comment 36

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 64ab5d6649a3227a98c5808d3eaa8910d571d3a1 to:
v1-train: 1a42f4f7d3260d0b97565ed6b2add408aec72cb4