1061215 - [Messages][Refactoring] Improve handling of remaining chars counter

Status: RESOLVED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

*** Follow-up from bug 1048845 ***

Currently #messages-counter-label is used to display "MMS" label in case of MMS without subject or remaining chars counter in case of SMS with less then 20 characters left. For the latter ":after" pseudo element is used, that makes code, naming and styling a bit confusing.

Initial idea is to use #messages-counter-label (should be renamed to something more generic) to store either remaining chars counter or "MMS" label (via data-l10n-id attribute) without pseudo-elements.

There is a chance that in the scope of this bug code that manipulates with remaining chars counter will be moved to Compose.js.

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Hey Steve,

Asking for low priority review here :) Since it's refactoring patch, I'm glad to adopt any other your ideas to refactor this part!

Thanks!

Comment 2

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8484801 [details] [review]
GitHub pull request URL

Hey Julien,

Redirecting review request to you since Steve is on PTO. This one blocks our sprint bug 1067228.

Thanks!

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8484801 [details] [review]
GitHub pull request URL

Reviewed on github.

The main issue is that we need to uncouple the state updates from the dom updates.

Please request review when ready :)

Thanks !

Comment 4

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8484801 [details] [review]
GitHub pull request URL

It should be ready for review again :)

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8484801 [details] [review]
GitHub pull request URL

Some more work needed, please ask review when ready :)

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8484801 [details] [review]
GitHub pull request URL

Hey Julien,

Thanks for review! I've updated PR + I decided to add marionette test for char counter vs MMS label as it's behaviour is not really trivial and we're going to refactor that :)

Also I had to rebase my patch on the latest master as bug 836690 is landed.

Could you please take a look once again?

Thanks!

Comment 7

[Julien Wajsberg [:julienw]]

NI Steve for the question in https://github.com/mozilla-b2g/gaia/pull/23726/files#r17835976

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8484801 [details] [review]
GitHub pull request URL

It's nearly there, please just add also specific classes for both mms-label elements, so that we can target them directly too in CSS.

Also some nits in the integration test.

About the segment banner, I don't think we need to focus on this here especially given the fact that we wait for this patch for bug 1067228. Let's file a separate bug for this specific item and move forward.

Thanks, please request review again once you've done the latest change (in a separate commit too please :) ).

Comment 9

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> NI Steve for the question in
> https://github.com/mozilla-b2g/gaia/pull/23726/files#﷒0﷓

Reply on github, thanks for digging out the bug :)

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8484801 [details] [review]
GitHub pull request URL

r=me

thanks for this good work!

Comment 11

[Oleg Zasypkin [:azasypkin]]

Thanks for comprehensive review!

Master: https://github.com/mozilla-b2g/gaia/commit/01aa7a6f55b8b6104e828f1d2c5b2a58d9a515b8