Closed
Bug 1021608
Opened 10 years ago
Closed 10 years ago
[Messages] Report panel visual refresh
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(feature-b2g:2.2+, tracking-b2g:backlog)
RESOLVED
FIXED
2.1 S7 (24Oct)
People
(Reporter: julienw, Assigned: steveck)
References
Details
(Whiteboard: [sms-most-wanted][p(2.1S7)=1][p(2.1S6)=2][p(2.1S5)=2])
Attachments
(4 files, 4 obsolete files)
See attachment: https://bug1021387.bugzilla.mozilla.org/attachment.cgi?id=8435416 Looking at this screen, I can't help thinking a "resend" button would be useful.
Flags: needinfo?(jelee)
It is quite useful but not exactly necessary since we already have two ways to resend: (1) short tap on the message and there will be a dialogue box pop up asking user if they want to try send again, (2) long press on the message and select "Resend this message" from action sheet. Here's another idea, I am thinking getting rid of (1) and add a "resend" button right next to the "!" icon on messages that had failed to deliver, that's based on the assumption that resend is the most common action people would take after failure of delivery (rather than deleting the message). However, it might not be visually pleasing :p Hello Vicky, what do you think of the idea to add a "resend" button on messages that had failed to send? The "resend" button can be placed ex. right next to the "!" button.
Flags: needinfo?(vpg)
Flags: needinfo?(jelee)
Flags: needinfo?(felash)
Reporter | ||
Comment 2•10 years ago
|
||
Jenny, tapping the "!" already does a resend (but with a confirmation). Do you suggest to remove this confirmation ? Also, I thought the "resend" button on the "report" panel could be useful. It would not be the primary way to resend a message, but as a user, when I am on this screen and I want to resend the message, here are the actions I need to do: * I first need to close the panel * then I need to visually locate the message (again) * then I need to either longpress it or tap the "!" icon This is cumbersome. However, when I am on the "report" screen and see the message is in "error", it feels completely logical, from an UX point of view, to have a button to act on this error (ie: resend the message).
Flags: needinfo?(felash) → needinfo?(jelee)
(In reply to Julien Wajsberg [:julienw] from comment #2) > Jenny, tapping the "!" already does a resend (but with a confirmation). Do > you suggest to remove this confirmation ? > Yes I was originally suggesting putting a resend button (in text) on screen directly next to the "!" button (this is similar to when you failed to send a picture with whatsapp, there will be an upload icon on the picture for you to 1 tap resend), but it doesn't work after I actually tried to layout. So let's keep the short tap and confirm to resend behavior. > However, when I am on the "report" screen and see the message is in "error", > it feels completely logical, from an UX point of view, to have a button to > act on this error (ie: resend the message). I'm totally okay with adding the "resend" button on report page. I was just assuming people would normally try to resend before they go to the report page, so resending is probably not what they are looking for on report page. However, as you pointed out, it can be very helpful, I'll update the wireframe accordingly. Tks!
Flags: needinfo?(vpg)
Flags: needinfo?(jelee)
Comment 4•10 years ago
|
||
Hi jeeny and Juliew @jenny I think what you told in comment 1 is right.We should remove (1). First reason :most of actions now run (reference to android) on long press rather than short tap.so user get used to it. second :for dialogue box we have tap on red icon(!) instead of error message, which is very small and not that user friendly. So i think we should remove this and (2) i.e Long press is quite enough as long press become general now a days. @juliew a/c to my POV , user not frequently check 'report message option' because icon (!) is enough to tell them (except new/first-time user).We may add 'resend option' in 'report message' but it's not that much necessary. Thanks
Reporter | ||
Comment 5•10 years ago
|
||
We don't check "report message panel" because we want to check the status, but to check other things (like size, etc); but _then_ we might want to resend asap.
Reporter | ||
Comment 8•10 years ago
|
||
Hey Fang, I don't remember if you've sent a design spec for message report somewhere else?
Flags: needinfo?(fshih)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [sms-most-wanted]
Comment 9•10 years ago
|
||
Per product prioritization, this could be 2.2 focus.
blocking-b2g: 2.1? → backlog
feature-b2g: --- → 2.2
Comment 10•10 years ago
|
||
Attached the visual spec for message report, Thanks!
Flags: needinfo?(fshih)
Reporter | ||
Updated•10 years ago
|
Blocks: sms-sprint-2.1S5
Whiteboard: [sms-most-wanted] → [sms-most-wanted][p=2]
Target Milestone: --- → 2.1 S5 (26sep)
Comment 13•10 years ago
|
||
Hey Anusha, Thanks for expressing interest! We've already planned this bug for the SMS team itself for the current sprint, and it's not really good first bug :) We would be happy if you can choose something that you like from list of our mentored bugs [1] [1] https://bugzilla.mozilla.org/buglist.cgi?f10=bug_mentor&f1=OP&list_id=11182777&f0=OP&f8=CP&v3=gaia%3A%3Asms&v10=felash%40gmail.com%2C%20schung%40mozilla.com%2C%20azasypkin%40mozilla.com&emailtype1=substring&f9=CP&emailassigned_to1=1&query_format=advanced&o10=anywordssubstr&j1=OR&bug_status=UNCONFIRMED&bug_status=NEW&email1=nobody%40mozilla.org&component=Gaia%3A%3ASMS Thanks!
Comment 14•10 years ago
|
||
Hey Oleg, Thank you for the reply, Will look into the mentored sms app related bugs.
Reporter | ||
Updated•10 years ago
|
Summary: [Messages] Consider adding a "resend" button in the message report page, if there is an error → [Messages] Consider adding a "resend" button in the message report page, if there is an error, and other visual refresh
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → schung
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•10 years ago
|
||
Hi julien, it's the first WIP for report view refresh. The layout change and resend button functionality is ready. The testing part is still ongoing and we might still got some concern in UX spec, but any early feedback would be really helpful :)
Attachment #8496079 -
Flags: feedback?(felash)
Reporter | ||
Comment 16•10 years ago
|
||
Comment on attachment 8496079 [details]
Link to github
Gave a first feedback but I didn't look on the device yet so keeping the feedback request
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8496079 [details]
Link to github
left comments on github :) looks good !
Attachment #8496079 -
Flags: feedback?(felash) → feedback+
Reporter | ||
Updated•10 years ago
|
Summary: [Messages] Consider adding a "resend" button in the message report page, if there is an error, and other visual refresh → [Messages] Report panel visual refresh
Reporter | ||
Updated•10 years ago
|
Blocks: sms-sprint-2.1S6
Whiteboard: [sms-most-wanted][p=2] → [sms-most-wanted][p(2.1S6)=2][p(2.1S5)=2]
Reporter | ||
Updated•10 years ago
|
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
Reporter | ||
Comment 18•10 years ago
|
||
I'm away for 3 days so please request review from oleg once you're ready !
Assignee | ||
Comment 19•10 years ago
|
||
Hi Oleg, sorry that I create another branch for it because I added unit test and some fixing after julien's 1st review. But I could not reopen the branch after some force update. The new patch is based on julien's feedback without test review yet, I'll add some comments for the new fixing, and really sorry for the inconvenience.
Attachment #8499695 -
Flags: review?(azasypkin)
Comment 20•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
Looks good, left some comments on GitHub, mostly just nits. Also it would be great to confirm with VD some changes that doesn't look similar to the spec.
Thanks!
Attachment #8499695 -
Flags: review?(azasypkin)
Assignee | ||
Comment 21•10 years ago
|
||
Hi Fang, here is the report panel screenshots for each cases. Oleg has some concern about the list layout that does not match the visual spec you provided. So it's safer to request review here since the original visual spec might need some changes as we discussed offline before, thanks.
Attachment #8502320 -
Flags: ui-review?(fshih)
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
Hi Oleg, thanks for the review. I've addressed most of comments and will leave some comments on github about the changes. I'll also attach images for ui-review for sure.
Attachment #8499695 -
Flags: review?(azasypkin)
Comment 23•10 years ago
|
||
Comment on attachment 8502320 [details]
report-screenshots.zip
Hi Steve,
Overall it looks good. I just need you to make some adjustments to the line height. The spacing between each text is too big right now. Can we reduce the spacing to smaller size for each text?
And for the contact name, currently we have it "bold", I would like to change to "400"(medium) so is closer to the spec. For the contact photo, they need to be vertically center aligned.
Please let me know if you have any questions. Thanks!
Attachment #8502320 -
Flags: ui-review?(fshih) → ui-review-
Assignee | ||
Comment 24•10 years ago
|
||
Hi Fang, I adjusted the layout per your suggestion, could you please review again? Thanks.
Attachment #8502438 -
Flags: ui-review?(fshih)
Comment 25•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
r=me, thanks! Just a few nits and question on Github.
Attachment #8499695 -
Flags: review?(azasypkin) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
Thanks for point out some critical issues in the patch, and I think it's necessary to request a review again for the chages, sorry for the inconvenience...
Attachment #8499695 -
Flags: review+ → review?(azasypkin)
Reporter | ||
Comment 27•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
Since Oleg is in PTO, I'll have a look shortly
Attachment #8499695 -
Flags: review?(azasypkin) → review?(felash)
Reporter | ||
Comment 28•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
left my comments about the race issue resolution and the unit test change.
I really think we should avoid the race instead of adding complexity to resolve it. Please tell me if you disagree, in that case we could go on and land this, and discuss this in a separate bug.
Attachment #8499695 -
Flags: review?(felash)
Comment 29•10 years ago
|
||
Hi Steve, Thanks for the updated! It's really close! : D For some reason the spacing of the phone number one (not in contact list) is still too large compared to the other list. Can we make all the spacing in same size? Also, can we move up the icons by 2 px vertically. Currently it looks lower than text. You can refer to the file attached for more details. Thanks!
Updated•10 years ago
|
Attachment #8502438 -
Flags: ui-review?(fshih) → ui-review-
Assignee | ||
Comment 30•10 years ago
|
||
Hi Fang, I've updated per your request in the latest patch.
Attachment #8502320 -
Attachment is obsolete: true
Attachment #8502438 -
Attachment is obsolete: true
Attachment #8503980 -
Attachment is obsolete: true
Attachment #8504020 -
Flags: ui-review?(fshih)
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #28) > Comment on attachment 8499695 [details] > Link to github > > left my comments about the race issue resolution and the unit test change. > > I really think we should avoid the race instead of adding complexity to > resolve it. Please tell me if you disagree, in that case we could go on and > land this, and discuss this in a separate bug. Since I could not figure out a simpler way to update the report block only, maybe we could apply your 1st proposal on github that reduce contact list rendering times? And sorry I'm not quite sure the unit test changes you mentioned here :-/
Reporter | ||
Comment 32•10 years ago
|
||
(In reply to Steve Chung [:steveck] from comment #31) > (In reply to Julien Wajsberg [:julienw] from comment #28) > > Comment on attachment 8499695 [details] > > Link to github > > > > left my comments about the race issue resolution and the unit test change. > > > > I really think we should avoid the race instead of adding complexity to > > resolve it. Please tell me if you disagree, in that case we could go on and > > land this, and discuss this in a separate bug. > > Since I could not figure out a simpler way to update the report block only, > maybe we could apply your 1st proposal on github that reduce contact list > rendering times? Yep, as said on github, let's do this :) > And sorry I'm not quite sure the unit test changes you > mentioned here :-/ I was mentioning the latest comment from Oleg :)
Assignee | ||
Comment 33•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
Adding the refinement for the rendering racing issue and unit test. The method I took might be slightly different than you suggested, but the goal is the same.
Attachment #8499695 -
Flags: review?(felash)
Reporter | ||
Comment 34•10 years ago
|
||
Comment on attachment 8499695 [details] Link to github > The method I took might be slightly different than you suggested, but the goal is the same. Sorry, but the method you took doesn't work here :(
Attachment #8499695 -
Flags: review?(felash)
Reporter | ||
Updated•10 years ago
|
Attachment #8496079 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
You are right about the edge cases, although it seems not possible to happen. I also add a test case for this scenario, thanks!
Attachment #8499695 -
Flags: review?(felash)
Updated•10 years ago
|
Attachment #8504020 -
Flags: ui-review?(fshih) → ui-review+
Reporter | ||
Updated•10 years ago
|
Blocks: sms-sprint-2.1S7
Whiteboard: [sms-most-wanted][p(2.1S6)=2][p(2.1S5)=2] → [sms-most-wanted][p(2.1S7)=1][p(2.1S6)=2][p(2.1S5)=2]
Reporter | ||
Updated•10 years ago
|
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (24Oct)
Reporter | ||
Comment 36•10 years ago
|
||
(In reply to Steve Chung [:steveck] from comment #35) > Comment on attachment 8499695 [details] > Link to github > > You are right about the edge cases Just want to point out that your previous proposal was not failing in an edge case only, but when requesting several times a report for a group MMS (with several recipients). > although it seems not possible to > happen. I also add a test case for this scenario, thanks!
Reporter | ||
Comment 37•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
Sorry, some nits that I didn't see in my previous review, my fault :/
2 nits are more important than others:
* you need to change the l10n key when you change the value.
* when we're in the "group view" panel, we're also still in the thread. I don't think this mistake has any consequence in the current code, but it can have later if the function is wrong, when we'll do group MMS right.
Attachment #8499695 -
Flags: review?(felash)
Assignee | ||
Comment 38•10 years ago
|
||
Comment on attachment 8499695 [details]
Link to github
These 2 issues is critical, thanks for pointing out :)
Attachment #8499695 -
Flags: review?(felash)
Reporter | ||
Comment 39•10 years ago
|
||
Comment on attachment 8499695 [details] Link to github r=me with the last nit, provided the tests are green. Don't forget to squash :) I see a glitch when displaying the panel: the space at the top (just above "Text Message (SMS)") is increasing about 1 sec after the report is displayed. Also, I see that the "delivered" tick image flash at the same moment. I think the best is to track this in a separate bug, but we should fix it though. Also maybe this could be the issue with flex layout in bug 1081072, as I have an older Gecko build currently. Can you check this on a current build and possibly file a separate bug? Thanks !
Attachment #8499695 -
Flags: review?(felash) → review+
Assignee | ||
Comment 40•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #39) > Comment on attachment 8499695 [details] > Link to github > > r=me with the last nit, provided the tests are green. Don't forget to squash > :) > > I see a glitch when displaying the panel: the space at the top (just above > "Text Message (SMS)") is increasing about 1 sec after the report is > displayed. Also, I see that the "delivered" tick image flash at the same > moment. I think the best is to track this in a separate bug, but we should > fix it though. Also maybe this could be the issue with flex layout in bug > 1081072, as I have an older Gecko build currently. > > Can you check this on a current build and possibly file a separate bug? > Thanks ! After bug 1081072 landed, the glitch issue is gone. So let's wait for the gaia-try now
Assignee | ||
Comment 41•10 years ago
|
||
Gaia Try is green now, thanks for all the review! In master: 3ec94f448bb5c1c9c264896685c6ef77ab718c87
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 43•10 years ago
|
||
Hey Steve, I see 2 issues with the patch, can you file 1 follow-up ? 1. we miss one string value for "report-status-not-applicable". I think it happens when I don't have read/delivery report enabled, I send a MMS, and display the report for this outgoing MMS. Here is the error I get in the console: 10-30 18:21:13.824 1460 1460 W Messages: Content JS WARN: mozL10n: A non-existing entity requested: report-status-not-applicable I don't know, maybe that's on purpose because we don't display it? In that case maybe we can have a "null" in the REPORT_MAP so that we don't request the l10n value, just to not have a log when we don't need to? 2. The line "Mobile, ...." is cut in the bottom in that case. We can clearly see the "," is cut off, and same if the carrier or phone type part has a "g" (ex: Orange).
Flags: needinfo?(schung)
Assignee | ||
Comment 44•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #43) > Hey Steve, > > I see 2 issues with the patch, can you file 1 follow-up ? > > 1. we miss one string value for "report-status-not-applicable". I think it > happens when I don't have read/delivery report enabled, I send a MMS, and > display the report for this outgoing MMS. Here is the error I get in the > console: > > 10-30 18:21:13.824 1460 1460 W Messages: Content JS WARN: mozL10n: A > non-existing entity requested: report-status-not-applicable > > I don't know, maybe that's on purpose because we don't display it? In that > case maybe we can have a "null" in the REPORT_MAP so that we don't request > the l10n value, just to not have a log when we don't need to? > Ya you are right, we don't need l10n for not-applicable case. I will propose a patch for this. > 2. The line "Mobile, ...." is cut in the bottom in that case. We can clearly > see the "," is cut off, and same if the carrier or phone type part has a "g" > (ex: Orange). Ah, will address this in new patch as well, thanks!
Flags: needinfo?(schung)
Comment 45•10 years ago
|
||
let's keep the already-landed improvement in 2.2.
feature-b2g: 2.2? → 2.2+
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•