Closed
Bug 868218
Opened 11 years ago
Closed 11 years ago
[MMS][User Story] Handle not-downloaded messages in thread view
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect, P1)
Tracking
(blocking-b2g:leo+, b2g18 fixed)
People
(Reporter: gnarf, Assigned: gnarf)
References
()
Details
(Keywords: feature)
Attachments
(2 files, 2 obsolete files)
790.47 KB,
application/pdf
|
Details | |
28.04 KB,
patch
|
julienw
:
review+
|
Details | Diff | Splinter Review |
Pages 39, 44, 47, and 48 of this UX document describe how to display "not-downloaded" mms messages in the thread view.
Assignee | ||
Updated•11 years ago
|
Summary: [MMS][User Story] → [MMS][User Story] Handle not-downloaded messages in thread view
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gnarf37
Updated•11 years ago
|
blocking-b2g: --- → leo+
Priority: -- → P1
Comment 2•11 years ago
|
||
Please block mms-userstories for all the [MMS][User Story] bugs so that it's easier to keep track of them.
Blocks: mms-userstories
Updated•11 years ago
|
Flags: in-moztrap?
Updated•11 years ago
|
Whiteboard: [NO_UPLIFT]
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #751298 -
Flags: review?(felash)
Assignee | ||
Comment 4•11 years ago
|
||
note, this patch depends on bug 872725 's patch
Assignee | ||
Comment 5•11 years ago
|
||
Any chance we have strings/visual design for these new screens? (see the wireframes) I used a date format we had defined in shared that displays like "05/27/2013" if you are en_US, but adapts order for other locales. We need to define more shared date formats I think if we want to show it more like the wireframe, and I'm not really sure who to ask for those.
Flags: needinfo?(vpg)
Flags: needinfo?(dietrich)
Flags: needinfo?(aymanmaat)
Comment 6•11 years ago
|
||
Corey, you should try to reach Ayman by mail.
Comment 7•11 years ago
|
||
Comment on attachment 751298 [details] [diff] [review] patch Review of attachment 751298 [details] [diff] [review]: ----------------------------------------------------------------- more lots of small comments than big changes ::: apps/sms/index.html @@ +184,5 @@ > </a> > --> > </div> > + > + <div id="messages-expired-tmpl" class="hide"> If I understand correctly, this will be used for also not-expired messages, therefore I'd like a another name for this template. ::: apps/sms/js/message_manager.js @@ +336,5 @@ > return this._mozMobileMessage.getMessage(id); > }, > > + retrieveMMS: function mm_retrieveMMS(id) { > + return this._mozMobileMessage.retrieveMMS(id); I'm concerned that we leak out of the abstraction here (ie: the DomRequest). I know we do this for getMessage too but still. But we can leave this for a future promise-related patch maybe. ::: apps/sms/js/thread_ui.js @@ +35,5 @@ > CONVERTED_MESSAGE_DURATION: 3000, > recipients: null, > init: function thui_init() { > var _ = navigator.mozL10n.get; > + var templateIds = ['contact', 'expired', 'highlight', 'message', 'recipient']; nit: seems to be a too long line @@ +848,5 @@ > } > > + if (notDownloaded) { > + // default strings: > + var messageString = 'not-downloaded' nit: you miss a semicolon at the end @@ +851,5 @@ > + // default strings: > + var messageString = 'not-downloaded' > + var download = 'not-downloaded-download'; > + > + var status = message.deliveryStatus[0]; for multi-recipient stuff, we'll have to change this, do we already have a bug for this ? @@ +863,5 @@ > + classNames.push('expired'); > + messageString = 'not-downloaded-expired'; > + } > + > + console.log('not-downloaded', status); remove the log @@ +867,5 @@ > + console.log('not-downloaded', status); > + > + if (status === 'error') { > + classNames.push('error'); > + } nit: add an empty line here @@ +1055,5 @@ > } > > // Click event handlers that occur outside of a message element should be > // defined elsewhere. > if (!elems.message) { you can do an early return for elems.bubble here, and simplify the 2 conditions below @@ +1255,5 @@ > } > }, > > + retrieveMMS: function thui_retrieveMMS(id) { > + if (typeof id !== 'number') { we don't need the if, just use `id = +id`. Also, to help the JIT compiler which doesn't like a single variable changing its type, we could create a new var to hold the value. @@ +1258,5 @@ > + retrieveMMS: function thui_retrieveMMS(id) { > + if (typeof id !== 'number') { > + id = +id; > + } > + console.log('retrieve', id); remove this log @@ +1262,5 @@ > + console.log('retrieve', id); > + > + var request = MessageManager.retrieveMMS(id); > + var messageDOM = this.container.querySelector('[data-message-id="' + id + > + '"]'); nit: put the selector on its own line, will make it easier to read. @@ +1267,5 @@ > + > + messageDOM.classList.add('pending'); > + messageDOM.classList.remove('error'); > + > + request.onsuccess = (function retrieveMMSSuccess() { we should handle errors too.
Attachment #751298 -
Flags: review?(felash)
Assignee | ||
Comment 8•11 years ago
|
||
All comments adressed in code - patch still requires bug 872725's patch first
Attachment #751298 -
Attachment is obsolete: true
Attachment #751372 -
Flags: review?(felash)
Comment 9•11 years ago
|
||
Comment on attachment 751372 [details] [diff] [review] patch Review of attachment 751372 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review. Some comments, but we would need UX input here for UI design. On the other hand, I miss some tests here. ::: apps/sms/index.html @@ +186,5 @@ > </div> > + > + <div id="messages-not-downloaded-tmpl" class="hide"> > + <!-- > + <span class="not-downloaded-message">${message}</span> Why a span? ::: apps/sms/js/thread_ui.js @@ +848,5 @@ > > + if (notDownloaded) { > + // default strings: > + var messageString = 'not-downloaded'; > + var download = 'not-downloaded-download'; Better to use 'not-downloaded-mms' and 'expired-mms'. It's more meaningful. @@ +851,5 @@ > + var messageString = 'not-downloaded'; > + var download = 'not-downloaded-download'; > + > + // assuming that incoming message only has one deliveryStatus > + var status = message.deliveryStatus[0]; We would need the input from UX for getting the design for this scenario, due to the design could differ from the original bubble. @@ +853,5 @@ > + > + // assuming that incoming message only has one deliveryStatus > + var status = message.deliveryStatus[0]; > + var expires = Utils.date.format.localeFormat( > + message.expiryDate, navigator.mozL10n.get('dateTimeFormat_%x') Probably here we should use the same rules as in the header (check 'getHeaderDate' function). Maybe we need some UX input here. @@ +1264,5 @@ > + }).bind(this); > + > + request.onerror = (function retrieveMMSError() { > + messageDOM.classList.remove('pending'); > + messageDOM.classList.add('error'); Is this the style needed for a non-retrieved MMS? Are we going to show an exclamation here? https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L424 ? I guess that we will need to add more styles here for 'incoming' MMS with different status in 'not-downloaded' scenario. ::: apps/sms/locales/sms.en-US.properties @@ +95,5 @@ > mms-message = MMS message > notDownloaded-title = Message is not downloaded yet. > +not-downloaded = I have sent you a file. It will be available until {{date}} > +not-downloaded-expired = Sorry, message cannot be downloaded as it expired on {{date}} > +not-downloaded-download = download 'download' instead? @@ +96,5 @@ > notDownloaded-title = Message is not downloaded yet. > +not-downloaded = I have sent you a file. It will be available until {{date}} > +not-downloaded-expired = Sorry, message cannot be downloaded as it expired on {{date}} > +not-downloaded-download = download > +not-downloaded-downloading = downloading 'downloading' instead? ::: apps/sms/style/sms.css @@ +319,5 @@ > } > > +/* hide download button via css on expired messages */ > +.message.expired button { > + display: none; Probably in 'expired' we dont need to render this element, because we are going to hide it after all.
Attachment #751372 -
Flags: review?(felash) → review?(fbsc)
Comment 10•11 years ago
|
||
Comment on attachment 751372 [details] [diff] [review] patch Review done. Let me know when the changes will be ready and I will review it again! Thanks.
Attachment #751372 -
Flags: review?(fbsc)
Comment 11•11 years ago
|
||
Ideally the phone would allow the user to localize/customize the date/time format, but to the best of my knowledge it does not. That said, the most widely used date format in the phone is the en_US one, and indeed this is the one that is used on the inbox of the messaging app, so i would use that and ignore the wireframes simply for the sake of consistency. But it must be noted that the en_US date format that will not fly in the european market, so user localization/customization of Date/Time format needs to be addressed and Moz Product should be aware of that. Let me know if you need more clarification
Flags: needinfo?(aymanmaat)
Comment 12•11 years ago
|
||
Ayman, we can localize whatever you want :)
Assignee | ||
Comment 13•11 years ago
|
||
adresses review comments from borja
Attachment #751372 -
Attachment is obsolete: true
Attachment #751687 -
Flags: review?(felash)
Attachment #751687 -
Flags: review?(fbsc)
Comment 14•11 years ago
|
||
note that I'll probably not review this today, but tomorrow for sure.
Comment 15•11 years ago
|
||
Re: date formatting - my recommendation is to ask someone like Fabrice or Vivien what the Gaia team has been doing for time/date localization so far, to be consistent with that.
Flags: needinfo?(vpg)
Flags: needinfo?(dietrich)
Comment 16•11 years ago
|
||
Comment on attachment 751687 [details] [diff] [review] patch Review of attachment 751687 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/sms/js/thread_ui.js @@ +832,5 @@ > var classNames = ['message', message.type, delivery]; > + > + var notDownloaded = delivery === 'not-downloaded'; > + > + if (delivery === 'received' || notDownloaded) { What do you think of creating a separate data structure to describe how to map delivery values to message direction? Something like ``` var incomingDeliveries = { received: 1, 'not-downloaded': 1 }; classNames.push(incomingDeliveries[delivery] ? 'incoming' : 'outgoing'); ``` (Of course, you could define that hash outside of this method, too.) You might use an array and `indexOf` instead, or maybe a full if/else instead of a ternary, but my thinking is that any such approach would be a little more explicit/readable. @@ +854,5 @@ > + var messageString = 'not-downloaded-mms'; > + > + // assuming that incoming message only has one deliveryStatus > + var status = message.deliveryStatus[0]; > + var expires = Utils.date.format.localeFormat( How about `expiryStr` instead of `expires`? I got a little lost reading through this, between `expired`, `expires`, `expired-mms`, and `expiryDate`.
Comment 17•11 years ago
|
||
(In reply to Mike Pennisi [:jugglinmike] from comment #16) > Comment on attachment 751687 [details] [diff] [review] > patch > > Review of attachment 751687 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: apps/sms/js/thread_ui.js > @@ +832,5 @@ > > var classNames = ['message', message.type, delivery]; > > + > > + var notDownloaded = delivery === 'not-downloaded'; > > + > > + if (delivery === 'received' || notDownloaded) { > > What do you think of creating a separate data structure to describe how to > map delivery values to message direction? Something like > > ``` > var incomingDeliveries = { received: 1, 'not-downloaded': 1 }; > > classNames.push(incomingDeliveries[delivery] ? 'incoming' : 'outgoing'); > ``` > > (Of course, you could define that hash outside of this method, too.) You > might use an array and `indexOf` instead, or maybe a full if/else instead of > a ternary, but my thinking is that any such approach would be a little more > explicit/readable. > The fact is that he's reusing the "notDownloaded" var later, and that's why he's defining it here. Maybe simply renaming "notDownloaded" to "isContentNotDownloaded" would be more readable. I think creating a hash or array for this is really overkill here.
Assignee | ||
Comment 18•11 years ago
|
||
(In reply to Mike Pennisi [:jugglinmike] from comment #16) > What do you think of creating a separate data structure to describe how to > map delivery values to message direction? Something like I had done an indexOf in an array at first but then realized i wanted a notDownloaded boolean later and switched back to the or conditional. > > + var expires = Utils.date.format.localeFormat( > > How about `expiryStr` instead of `expires`? I got a little lost reading > through this, between `expired`, `expires`, `expired-mms`, and `expiryDate`. Good call, I will change to expireFormatted before this lands
Updated•11 years ago
|
Comment 19•11 years ago
|
||
Comment on attachment 751687 [details] [diff] [review] patch r=me thanks !
Attachment #751687 -
Flags: review?(felash) → review+
Assignee | ||
Comment 20•11 years ago
|
||
master: 1c0188f9440de1262c7c6a0d3c75d165f7f1008d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Attachment #751687 -
Flags: review?(fbsc)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [NO_UPLIFT]
Flags: in-moztrap? → in-moztrap+
You need to log in
before you can comment on or make changes to this bug.
Description
•