1008127 - [Messages][Refresh] Subject handling in the Composer

Status: RESOLVED FIXED

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

Description

[Noemí Freire (:noemi)]

The goal of this bug is to handle subject in the composer as described in the visual spec:
https://bug963018.bugzilla.mozilla.org/attachment.cgi?id=8417465

covering the following scenarios:
*long text + subject empty
*long text + subject filled up

Comment 1

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

Hey all,
I made a patch who cover almost all of the spec with some small things that need to be discused.

First of all about the spec:


No Subject (MMS)
================================================================================
1. When we have one line message counter is not visible for the user (right?)
2. When we have two or more lines of the message the counter is visible and is aligned with the top of the message (if we follow the spec when we have 2 lines the counter goes under the `Send` button. That's why i move the counter a bit up.

With Subject (SMS)
================================================================================
1. Here we have the 'MMS' instead of 'Counter' and the MMS text is aligned with the bottom of the Subject with bottom border with color #9ef2e7

Noemí, can you confirm this?


Julien :),
if Noemí is OK with this, I think that we need only few things to be done in to JS logic:
================================================================================
1. Maybe we need to change data-counter on #messages-input only when we don't have subject
2. Maybe we need to remove the code who add/remove/toggle the `has-counter` class of #messages-send-button element

I think that's all.
Hope this help all of us :)

Comment 2

[Julien Wajsberg [:julienw]]

Historically, we display the counter when we have less than 20 characters left in the first message section (it means that we have 20 characters left before sending 2 SMS).

I think we want to keep it that way, but Omega will confirm.

Comment 3

[Omega Feng [:Omega] [:馮於懋]]

ni? new UX owner Jenny.

Comment 4

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Historically, we display the counter when we have less than 20 characters
> left in the first message section (it means that we have 20 characters left
> before sending 2 SMS).
> 
> I think we want to keep it that way, but Omega will confirm.

Correct, we want to keep it that way.
BTW, I also found two issue in the patch.
1) The counter seems to stay 82/1 always and never change.
2) The counter can be scroll outside of the screen.
Please also fix these issues, thanks.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

I haven't tried (this needs a rebase now that bug 963018 landed) but the general code looks right.

Comment 6

[Julien Wajsberg [:julienw]]

Pavel, please ask a review from :steveck if you need a review before the end of the week.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

I don't think it's ready for review yet.

Pavel, I think you need someone to work with you to finish this, especially on the JS side.

Oleg, can you do the needed JS changes here? See comment 1 for more information and don't hesitate to ask me on IRC tomorrow if necessary.

Comment 8

[Julien Wajsberg [:julienw]]

Vicky, Pavel has some questions for you in comment 1, can you please have a look? Thanks !

Comment 9

[Victoria Gerchinhoren [:vicky]]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #1)
> Created attachment 8427759 [details] [review]
> patch for Gaia/master
> 
> Hey all,
> I made a patch who cover almost all of the spec with some small things that
> need to be discused.
> 
> First of all about the spec:
> 
> 
> No Subject (MMS)
> =============================================================================
> ===
> 1. When we have one line message counter is not visible for the user (right?) 
Right

> 2. When we have two or more lines of the message the counter is visible and
> is aligned with the top of the message (if we follow the spec when we have 2
> lines the counter goes under the `Send` button. That's why i move the
> counter a bit up.
> 
> With Subject (SMS)
> =============================================================================
> ===
> 1. Here we have the 'MMS' instead of 'Counter' and the MMS text is aligned
> with the bottom of the Subject with bottom border with color #9ef2e7
> 
> Noemí, can you confirm this?
> 
This is correct. When you haven't started typing the subject, the line is shorter, and when the user starts typing it, the line grows reaching the full width and the MMS indicator appears above it. This is because I think is good to give the user a hint of why this message is turning into an MMS (because of the subject addition)
> 
> Julien :),
> if Noemí is OK with this, I think that we need only few things to be done in
> to JS logic:
> =============================================================================
> ===
> 1. Maybe we need to change data-counter on #messages-input only when we
> don't have subject
> 2. Maybe we need to remove the code who add/remove/toggle the `has-counter`
> class of #messages-send-button element
> 
> I think that's all.
> Hope this help all of us :)

Thanks!

Comment 10

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Thanks all,
we need JS help for this one for sure :) and if you need my help don't hesitate to ask me on IRC or email

Comment 11

[Oleg Zasypkin [:azasypkin]]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #10)
> Thanks all,
> we need JS help for this one for sure :) and if you need my help don't
> hesitate to ask me on IRC or email

Hey Pavel and Julien, don't worry I'm looking into it :)

Comment 12

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (JS part)

So, here is what I think we need from JS side. But we still need few things that I believe we can do with CSS:

Omega's suggestions:
1. Show counter only when <= 20 available characters left, so you can use 'has-counter' class as earlier;
2. The counter can be scrolled outside of the screen.

Victoria suggestion:
> When you haven't started typing the subject, the line is
> shorter, and when the user starts typing it, the line grows reaching the
> full width and the MMS indicator appears above it. This is because I think
> is good to give the user a hint of why this message is turning into an MMS
> (because of the subject addition) 

Pavel, please let me know whether JS updates work for you and if so, I'll ask for code review.

Thanks

Comment 13

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

LGTM :)
BTW: I update my patch because of conflicts in sms.css

Comment 14

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Oleg,
as we chat on IRC I made the changes and I update my PR

The only one thing is that we need to add `has-counter` class on `<div class="message-complete">` when user start typing a message ... this will show the MMS/counter :) and also stretch the blue border of the subject

ping me when you add the JS part and I can f+ your PR

Thanks again :)

Comment 15

[Oleg Zasypkin [:azasypkin]]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #14)
> Hey Oleg,
> as we chat on IRC I made the changes and I update my PR
> 
> The only one thing is that we need to add `has-counter` class on `<div
> class="message-complete">` when user start typing a message ... this will
> show the MMS/counter :) and also stretch the blue border of the subject
> 
> ping me when you add the JS part and I can f+ your PR
> 
> Thanks again :)

Hey Pavel,

Not sure that adding "has-counter" when user starts typing a message is the right thing. We add "has-counter" only for SMS and only when we have <= 20 chars left, so it can be used to show\hide available chars counter.

Regarding to the 'MMS' label - as it's shown for MMS only, "[data-message-type="mms"]" selector was used to detect that case previously. Initially we have "[data-message-type="sms"]", and change it to "[data-message-type="mms"]" when user converts message to mms. If we show subject field, but it's empty - it's still SMS, but once user enters something into subject field then type is changed to MMS. We show "MMS" label and extend bottom border (looks like it's just manipulating with subject's input margin-right?) when we have "[data-message-type="mms"]" on the parent container. 

We already have all this in JS part, so looks like we need to adjust CSS only. 

What do you think?

Comment 16

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Ok ... my PR is updated now ... can you update yours with adding `has-counter` on `#messages-subject-input` when use start typing the subject. and then we will ask Vicky for opinion because I saw few cases when we have `MMS` label over the blue border and under the blue border ... I think this is a mistake because otherwise we will need to change a lot of stuff there.

Comment 17

[Julien Wajsberg [:julienw]]

Pavel, can you base your work here on the code of bug 1015867 ?

I change how the composer is layed out and I think this will change some of the things you do here.

Comment 18

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Sure ...

Comment 19

[Oleg Zasypkin [:azasypkin]]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #16)
> Ok ... my PR is updated now ... 
Hmmm, I still see that available chars counter can go off screen when you type long message, so you have to scroll the content to see how much characters left.            

> can you update yours with adding `has-counter` on `#messages-subject-input` when use start typing the
> subject. 
Sorry it's not clear, why do you need to ".has-counter" for #messages-subject-input? These two are unrelated. Can you just use "[data-message-type="mms"]" for this case, I mean when user starts typing subject message it's converted to mms anyway? At the same time I don't see that you use ".has-counter" to show\hide available chars counter.

> and then we will ask Vicky for opinion because I saw few cases when
> we have `MMS` label over the blue border and under the blue border ... I
> think this is a mistake because otherwise we will need to change a lot of
> stuff there.

I think it would be better to ask Vicky as soon as possible :)

Please, ping me on IRC if you need more details.

Thanks!

Comment 20

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Vicky,

what happened when we have a long message? where the counter should be? Also I saw in the spec two cases the first one is when we have `MMS` over the blue border and the second one is when the `MMS` is under the border?

Comment 21

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I also update my PR based on Oleg's comment about `[data-message-type="mms"]` so we just need info from Vicky about my previews comment

Comment 22

[Oleg Zasypkin [:azasypkin]]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #21)
> I also update my PR based on Oleg's comment about
> `[data-message-type="mms"]` so we just need info from Vicky about my
> previews comment

Hey Pavel, looks like something went wrong when you were rebasing your last PR, sms.css in the PR contains a lot of changes that it shouldn't. Could you please check?

Thanks!

Comment 23

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Sorry my mistake. Now is OK

Comment 24

[Victoria Gerchinhoren [:vicky]]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #20)
> Hey Vicky,
> 
> what happened when we have a long message? where the counter should be? Also
> I saw in the spec two cases the first one is when we have `MMS` over the
> blue border and the second one is when the `MMS` is under the border?

Hi Pavel,
MMS indicator should always be over that line. When there's a subject, there's an MMS indicator. 
If there's no subject, the cpunter and MMS indicator should be vertically aligned to the top of the input area. All this I said a few times already, Joan, Arnau and Oleg, who's taking care of this?

Thanks.

Comment 25

[Oleg Zasypkin [:azasypkin]]

Attachment: sms-subject-and-counter-visible.png

(In reply to Victoria Gerchinhoren [:vicky] from comment #24)
> (In reply to Pavel Ivanov [:ivanovpavel] from comment #20)
> > Hey Vicky,
> > 
> > what happened when we have a long message? where the counter should be? Also
> > I saw in the spec two cases the first one is when we have `MMS` over the
> > blue border and the second one is when the `MMS` is under the border?
> 
> Hi Pavel,
> MMS indicator should always be over that line. When there's a subject,
> there's an MMS indicator. 
> If there's no subject, the cpunter and MMS indicator should be vertically
> aligned to the top of the input area. All this I said a few times already,
> Joan, Arnau and Oleg, who's taking care of this?
> 
> Thanks.

Hey Victoria,

Thanks for your reply! We're all trying to take care of this :) Though there is one more case that I'd like to confirm with you:

In case we have empty and visible subject input we don't display MMS per spec (cause it's still SMS) and <= 20 avaialable chars left. Should we display available chars counter aligned to the top of the input area (below subject input). I've attached screenshot for you.

Thanks!

Comment 26

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I work on it ... we just land few patches who I need to rebase and Oleg and I work on it. Hope I will post a patch today

Comment 27

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

Hey Oleg,
as we talk few hours ago ... I made totally new patch ... and I think it works perfect :) you can also check our patches together here (https://github.com/pivanov/gaia/tree/pr-19598-subject-vr)

Comment 28

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

(In reply to Pavel Ivanov [:ivanovpavel] from comment #27)
> Comment on attachment 8427759 [details] [review]
> patch for Gaia/master
> 
> Hey Oleg,
> as we talk few hours ago ... I made totally new patch ... and I think it
> works perfect :) you can also check our patches together here
> (https://github.com/pivanov/gaia/tree/pr-19598-subject-vr)

Hey Pavel! 

I've just briefly tested your patch on device and subject part works great except that you need to use 'has-counter' to show counter-label only when this class is applied + '[data-message-type='mms']' to hide it totally;

Also I've noticed that message input behavior has changed - it doesn't occupy that much empty space as before. You can just compare how it looked before and how it looks after your patch is applied e.g. if you open message draft with very long message (3-4 sms) especially if you hide keyboard.

I'm on holiday today, so you can ask for feedback from Julien who made message input to occupy as much empty space as possible btw :)

Thanks!

Comment 29

[Julien Wajsberg [:julienw]]

I think it's better if Steve reviews this, as I'm not here next week and I expect this will need several rounds.

Comment 30

[Stephany Wilkes]

Pavel, what is the status? Please update.

If Julien has been on holiday, does someone else need to review this? We are now two weeks past feature landing and we need to make a decision on whether or not to block ship on 2.0 on this. No more feature work should be happening right now.

Oleg, can you do anything to help move this along or to give a judgment call? We need to make a call by tomorrow.

Comment 31

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Steph,
Oleg and I work on it and I will do my best to have this patch till end of the day.

Comment 32

[Stephany Wilkes]

Thank you, Pavel!

Comment 33

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: this PR contains Oleg and Pavel patches

Hey Oleg ... I think that I cover everything now ... can you check it before we ask for r+?

Comment 34

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8445339 [details] [review]
this PR contains Oleg and Pavel patches

Great job! I don't see any issues now. So let me update unit tests in JS part and I think we'll be ready for review.

Thanks!

Comment 35

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Thanks Oleg :) When you are ready I will update my patch with my changes and you can update yours ... or ... we will push everything like one PR?

Comment 36

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub branch URL (JS part)

(In reply to Pavel Ivanov [:ivanovpavel] from comment #35)
> Thanks Oleg :) When you are ready I will update my patch with my changes and
> you can update yours ... or ... we will push everything like one PR?

Hey Pavel,

I've finished with JS/unit tests changes in my branch (see attached URL).

Thanks

Comment 37

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

Hey Oleg,
I update my patch (branch ) and you can f+ if you are ok and also you can remove following files from your patch:
apps/sms/index.html
apps/sms/style/composer.css

Thanks :)

Comment 39

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

Hey Steve,
this PR contains all changes for this bug.

Comment 40

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

Thanks Pavel, it looks good on devices I have, I've also quickly skimmed through the code, I think it would be nice to have some clarification comments for the most important numbers used in CSS (especially for numbers used in "calc" functions).

Thanks!

Comment 41

[Steve Chung [:steveck]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

I left some comments and question for Oleg. The new visual works really great, thanks!

Comment 42

[Steve Chung [:steveck]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

Hi Pavel/Oleg, the patch seems got some conflicts(might related to patch in bug 1013296), could you please fix them for another review? Thanks!

Comment 43

[Steve Chung [:steveck]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

Looks great now! r=me and thanks for the all the efforts. Travis is still red but we already got the corresponding bugs(bug 1032288 for vertical home and bug 1032037 for music), it should be fine for merging.

Comment 44

[Steve Chung [:steveck]]

In master: 331250d734e6f2539f1fab8ba6e9e0e3fcbe2172

Comment 45

[Steve Chung [:steveck]]

Comment on attachment 8427759 [details] [review]
patch for Gaia/master

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): new feature
[User impact] if declined: not following visual refresh targeted for 2.0
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: No

Since this feature is part of visual refresh and affected bug 990537, requesting approval for v2.0

Comment 46

[Julien Wajsberg [:julienw]]

Note that it's part of the visual refresh but also fixes and polishes the issues that the previous visual refresh patches brought. Without this patch in v2.0, we'll likely have blockers to fix things this patch is already fixing.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/cfa7fa4b4c861069acede3459028a46d4b01b949