Closed
Bug 1061215
Opened 10 years ago
Closed 10 years ago
[Messages][Refactoring] Improve handling of remaining chars counter
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: azasypkin, Assigned: azasypkin)
References
Details
(Whiteboard: [sms-papercuts][sms-sprint-2.1S5])
Attachments
(1 file)
*** 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.
Assignee | ||
Comment 1•10 years ago
|
||
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!
Attachment #8484801 -
Flags: review?(schung)
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8484801 [details]
GitHub pull request URL
Hey Julien,
Redirecting review request to you since Steve is on PTO. This one blocks our sprint bug 1067228.
Thanks!
Attachment #8484801 -
Flags: review?(schung) → review?(felash)
Comment 3•10 years ago
|
||
Comment on attachment 8484801 [details]
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 !
Attachment #8484801 -
Flags: review?(felash)
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8484801 [details]
GitHub pull request URL
It should be ready for review again :)
Attachment #8484801 -
Flags: review?(felash)
Comment 5•10 years ago
|
||
Comment on attachment 8484801 [details]
GitHub pull request URL
Some more work needed, please ask review when ready :)
Attachment #8484801 -
Flags: review?(felash)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8484801 [details]
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!
Attachment #8484801 -
Flags: review?(felash)
Comment 7•10 years ago
|
||
NI Steve for the question in https://github.com/mozilla-b2g/gaia/pull/23726/files#r17835976
Flags: needinfo?(schung)
Comment 8•10 years ago
|
||
Comment on attachment 8484801 [details]
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 :) ).
Attachment #8484801 -
Flags: review?(felash)
Comment 9•10 years ago
|
||
(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 :)
Flags: needinfo?(schung)
Comment 10•10 years ago
|
||
Comment on attachment 8484801 [details]
GitHub pull request URL
r=me
thanks for this good work!
Attachment #8484801 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
Thanks for comprehensive review!
Master: https://github.com/mozilla-b2g/gaia/commit/01aa7a6f55b8b6104e828f1d2c5b2a58d9a515b8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [sms-papercuts] → [sms-papercuts][sms-sprint-2.1S5]
You need to log in
before you can comment on or make changes to this bug.
Description
•