Closed Bug 1045499 Opened 10 years ago Closed 10 years ago

Switch call log to a 2-line layout

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.1, tracking-b2g:backlog)

RESOLVED FIXED
2.1 S3 (29aug)
feature-b2g 2.1
tracking-b2g backlog

People

(Reporter: cawang, Assigned: drs)

References

Details

(Whiteboard: [planned-sprint c=3][in-sprint=v2.1-S2])

Attachments

(8 files, 9 obsolete files)

412.03 KB, image/jpeg
Details
62.21 KB, image/png
Details
296.95 KB, image/jpeg
Details
6.13 KB, patch
rik
: review+
Details | Diff | Splinter Review
66.23 KB, image/png
cawang
: ui-review+
Carol
: ui-review+
Details
37.47 KB, patch
rik
: review+
Details | Diff | Splinter Review
216.38 KB, image/png
cawang
: ui-review+
Details
110.69 KB, image/jpeg
Details
While designing the US 877971, we found the current implementation does not match the original UX spec. Hence, we did some evaluation and provide some new rules on the page. Please take a look and let me know if I've missed anything here. Thanks!
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
Depends on: 1045515
- For the unknown calls, we don't have a type of number ("Mobile") to display on the second row, what should we do? - I've split the missed call highlighting in bug 1045515. - We'll need some visual precisions for the height of each row and the right margin for the time of the call.
Summary: [Dialer] Call log list view relayout → Switch call log to a 2-line layout
Forgot to needinfo Carrie for comment 2.
Flags: needinfo?(cawang)
Blocks: 1047351
Target Milestone: --- → 2.1 S2 (15aug)
Assignee: nobody → drs+bugzilla
Whiteboard: [planned-sprint c=3]
blocking-b2g: 2.1? → backlog
feature-b2g: --- → 2.1
it's a UI improvement which should not block release.
feature-b2g: 2.1 → ---
Hi Anthony, Can we detect something else, such as location of the call? (This is Android solution and I found it's quite helpful). If not, then we can just display "unknown". ni? Carol for visual spec. Thanks!
Flags: needinfo?(cawang) → needinfo?(chuang)
(In reply to Carrie Wang [:carrie] from comment #5) > Can we detect something else, such as location of the call? (This is Android > solution and I found it's quite helpful). If not, then we can just display > "unknown". I think this would require some kind of web services or library of area codes stored on the device, so I would venture to say it's not an option.
Attached image CallLog_2lines_spec.jpg (obsolete) —
Hi Anthony, the attachment is the visual spec changes for the call log. let me know if you have any question. Thanks!!
Flags: needinfo?(chuang) → needinfo?(anthony)
Thanks! Doug: Can you take a look today and see if you have questions for Carol?
Flags: needinfo?(anthony) → needinfo?(drs+bugzilla)
Attached image Overflow example (obsolete) —
A couple of questions: Is the number next to the name the retry count? I assume so since I think it would be bad to leave it next to the time field. If we overflow the type+carrier field as in the attached picture, should we just ellipsize it?
Flags: needinfo?(chuang)
Attached image CallLog_2lines_spec.jpg (obsolete) —
Hi Doug, Yes,I've moved Retry # next to the name. For the other question that you mentioned, I'm not sure about the screenshot you attached but I assumed that you mean the type+carrier text length. I add to the visual spec as below, -If the type+carrier is longer than 16.5 rem(w), the text need to be elipsized. (please see the attachment: CallLog_2lines_spec.jpg) Thanks! Let me know if you have any other question.
Attachment #8469084 - Attachment is obsolete: true
Flags: needinfo?(chuang) → needinfo?(drs+bugzilla)
Sorry, I uploaded the wrong file. You answered my question anyways, so I think that's everything we need. Thanks.
Status: NEW → ASSIGNED
Flags: needinfo?(drs+bugzilla)
Attachment #8469771 - Attachment is obsolete: true
Attached patch Fix JSHint errors. (obsolete) — Splinter Review
Attachment #8473473 - Flags: review?(anthony)
Attached image Screenshot, patch applied. (obsolete) —
Carol, I have a couple of things to note here: * The time on the far right looks too bunched up to me with the scrollbar, so I moved it slightly to the left. This looks much better to me on a device. * The spec was unclear on whether or not the time should be in red. I wasn't sure if that was for the sake of highlighting or because it should actually be red. Both of these are easy to change if we want to.
Attachment #8473476 - Flags: ui-review?(chuang)
Reference screenshot for comparison with attachment 8473476 [details].
Comment on attachment 8473476 [details] Screenshot, patch applied. Hi Doug, Everything looks good, just on little thing. Can you help me change the Call# text color to #bc1515 ? 'time' text color is good. I'll update the visual spec. Thank you!!!
Attachment #8473476 - Flags: ui-review?(chuang) → ui-review-
Flags: needinfo?(drs+bugzilla)
Attached image CallLog_2lines_spec.jpg
Visual spec update: Change the call number& name to red (#bc1515)
Attachment #8470596 - Attachment is obsolete: true
Updated PR.
Attachment #8473472 - Attachment is obsolete: true
Attachment #8473472 - Flags: review?(anthony)
Attachment #8473831 - Flags: review?(anthony)
Attachment #8473473 - Attachment is obsolete: true
Attachment #8473473 - Flags: review?(anthony)
Attachment #8473832 - Flags: review?(anthony)
Here it is with the changes listed in comment 17. Carrie, I'm flagging you to check the "Unknown" for unknown numbers.
Attachment #8473476 - Attachment is obsolete: true
Attachment #8473833 - Flags: ui-review?(chuang)
Attachment #8473833 - Flags: ui-review?(cawang)
Flags: needinfo?(drs+bugzilla)
Comment on attachment 8473833 [details] Screenshot, patch applied. Hi Doug, It looks pretty good to me!thank you!
Attachment #8473833 - Flags: ui-review?(chuang) → ui-review+
Whiteboard: [planned-sprint c=3] → [planned-sprint c=3][in-sprint=v2.1-S2]
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Comment on attachment 8473833 [details] Screenshot, patch applied. Looks great! I'm so happy!
Attachment #8473833 - Flags: ui-review?(cawang) → ui-review+
Comment on attachment 8473832 [details] [diff] [review] Fix JSHint errors. Review of attachment 8473832 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/communications/dialer/js/call_log.js @@ +917,3 @@ > case 'create': > case 'update': > + */ Why commenting this and not deleting?
Attachment #8473832 - Flags: review?(anthony) → review+
Comment on attachment 8473831 [details] [diff] [review] Switch call log to a 2-line layout. Review of attachment 8473831 [details] [diff] [review]: ----------------------------------------------------------------- Globally, this is the right direction. There's some more cleanup to do in this patch. Also, we need a followup bug to delete the photos from the CallLogDb. ::: apps/communications/dialer/js/call_log.js @@ -462,5 @@ > > label.appendChild(input); > label.appendChild(span); > > var aside = document.createElement('aside'); This element is now empty, we should remove it. @@ -466,5 @@ > var aside = document.createElement('aside'); > aside.className = 'pack-end'; > - var img = document.createElement('span'); > - img.dataset.type = 'img'; > - img.className = 'call-log-contact-photo'; You need to update the comment of createGroup to match the new DOM. @@ +511,5 @@ > + var addInfo = document.createElement('p'); > + addInfo.className = 'additional-info'; > + addInfo.setAttribute('aria-hidden', 'true'); > + > + var typeCarrier = document.createElement('span'); typeAndCarrier is probably a better name. I originally thought it was the type of carrier. @@ +522,4 @@ > var callTime = document.createElement('span'); > callTime.className = 'call-time'; > callTime.textContent = Utils.prettyDate(date) + ' '; > + if (addInfo) { No need for this if. ::: apps/communications/dialer/style/call_log.css @@ +196,4 @@ > } > > +.log-item .additional-info { > + text-transform: capitalize; I think this is frowned upon for L10n issues. Can you check with yzen? @@ +205,5 @@ > + overflow: hidden; > + text-overflow: ellipsis; > +} > + > +.log-item .additional-info .call-time { Most of the selectors above can use only the right most class and still work. ::: shared/js/dialer/utils.js @@ -68,5 @@ > */ > getPhoneNumberAdditionalInfo: > function ut_getPhoneNumberAdditionalInfo(matchingTel) { > - var number = matchingTel.number || matchingTel.value; > - if (!number) { This is a change in behaviour. Have you checked that all call sites (callscreen for example) are able to handle this new behaviour? Also, the mock should be updated to not return undefined anymore.
Attachment #8473831 - Flags: review?(anthony) → review-
Updated PR. The PR has a new commit for the review fixes. This is a rollup.
Attachment #8473831 - Attachment is obsolete: true
Attachment #8475531 - Flags: review?(anthony)
This raises a good question. Carrie, should the callscreen be updated to have the same "additional info" field as well? This used to contain the phone number of the contact, but now it contains the "type, carrier" combination, e.g. "Mobile, Telefonica". See attachment for an example.
Attachment #8475532 - Flags: ui-review?(cawang)
(In reply to Anthony Ricaud (:rik) from comment #24) > Why commenting this and not deleting? For reference so that onlookers can see the possible values. I'm not attached to this and would be fine with removing it.
Comment on attachment 8475532 [details] Screenshot, patch applied, call screen addl. info. Hi Doug, I wonder can we replace the carrier name on the call screen with the numbers (of course, only for existing contacts)? So for the existing contacts who have been set with carrier names in Contact details, we display: 1. number type + carrier name (as the second row) in Call log -> In this case, users can long-tap to check the numbers in call info. In addition, we don't have enough space to display numbers in call log and it does cause mess. 2. number type + numbers (as the second row) on call screen -> In this case, users might need some info to make sure which number he is calling to or calling him. What do you say? Thanks!
Attachment #8475532 - Flags: ui-review?(cawang) → ui-review-
Updated PR.
Attachment #8475531 - Attachment is obsolete: true
Attachment #8475531 - Flags: review?(anthony)
Attachment #8475986 - Flags: review?(anthony)
(In reply to Carrie Wang [:carrie] from comment #29) > Comment on attachment 8475532 [details] > Screenshot, patch applied, call screen addl. info. Yeah, this makes sense to me. I've implemented this in attachment 8475986 [details] [diff] [review] and you can see it in the attached screenshot.
Attachment #8475532 - Attachment is obsolete: true
Attachment #8475988 - Flags: ui-review?(cawang)
Comment on attachment 8475986 [details] [diff] [review] Switch call log to a 2-line layout. Review of attachment 8475986 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/callscreen/test/unit/handled_call_test.js @@ +540,5 @@ > }); > }); > }); > > + test('should display contact info', function() { Whoops. Will switch this back before I land this / after the next review cycle.
Attachment #8475986 - Flags: review?(anthony) → review+
Attachment #8475988 - Flags: ui-review?(cawang) → ui-review+
It looks really awesome! great job. Thanks!
Attached image text.jpg
hey Doug, I saw the screenshot you have and the contact name is too close to (number type + numbers). The contact name even got cropped. Could you add 0.5 rem space for it? Thank you!
Flags: needinfo?(drs+bugzilla)
Hi Doug, Could you just make sure they have 0.5 rem space between those two lines? Because the text on my device looks ok, didnt got cropped. Thank you :)
(In reply to Carol Huang [:Carol] from comment #34) > Created attachment 8476508 [details] > text.jpg > > hey Doug, > I saw the screenshot you have and the contact name is too close to (number > type + numbers). The contact name even got cropped. > Could you add 0.5 rem space for it? > Thank you! (In reply to Carol Huang [:Carol] from comment #35) > Hi Doug, > Could you just make sure they have 0.5 rem space between those two lines? > Because the text on my device looks ok, didnt got cropped. > Thank you :) Filed bug 1056883.
Flags: needinfo?(drs+bugzilla)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1056943
This should have the feature-b2g:2.1 flag. Bhavana, would you please add this? Thanks.
Flags: needinfo?(bbajaj)
feature-b2g: --- → 2.1
Flags: needinfo?(bbajaj)
See Also: → 818991
Depends on: 1078663
See Also: → 1089111
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: