Closed Bug 873498 Opened 11 years ago Closed 11 years ago

[MMS] [UX] Compose. Contacts suggestion should not show the contact's photo

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: vicky, Assigned: rwaldron)

References

Details

Attachments

(2 files)

Showing the picture makes the contacts suggestion list too busy and it will affect performance, so since it is not shown in the specs I suggest to be removed.
Blocks: 872514
Assignee: nobody → waldron.rick
Attached file Fix for 873498
Attachment #754170 - Flags: review?(felash)
blocking-b2g: --- → leo?
Comment on attachment 754170 [details] [review]
Fix for 873498

r=me with the nit and rebase
Attachment #754170 - Flags: review?(felash) → review+
Landed https://github.com/mozilla-b2g/gaia/commit/c7f5c27137cc36f51c9da194292e1e5055675017
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Rick, you made the same error than me when you rebased: you rolled back a previous change (see [1], and as a result, failed the tests.

Yay for the tests !

Please push a follow-up or I'll back out :)

[1] https://github.com/mozilla-b2g/gaia/commit/c7f5c27137cc36f51c9da194292e1e5055675017#L2R1390
Flags: needinfo?(waldron.rick)
Attached patch fix testsSplinter Review
This fixes the bad rebase, which reverted one line from Bug 872395, and made the tests fail.
Attachment #754541 - Flags: review?(gnarf37)
Flags: needinfo?(waldron.rick)
Comment on attachment 754541 [details] [diff] [review]
fix tests

Borja, could you please have a look to this one-liner fix so that we can fix the tests today ?

Note that I don't know how it comes that Travis is green now, the tests are always failing on my computer without this change...
Attachment #754541 - Flags: review?(fbsc)
Attachment #754541 - Flags: review?(schung)
Attachment #754541 - Flags: review?(gnarf37)
Attachment #754541 - Flags: review?(fbsc)
Comment on attachment 754541 [details] [diff] [review]
fix tests

Although it's a little bit confusing that having a " |" after the type string, I think it's ok only for interpolation only.
Attachment #754541 - Flags: review?(schung) → review+
(In reply to Steve Chung from comment #7)
> Comment on attachment 754541 [details] [diff] [review]
> fix tests
> 
> Although it's a little bit confusing that having a " |" after the type
> string, I think it's ok only for interpolation only.

That's what the wireframe says (maybe it's better to do this with border in CSS), and that was not my code ;)

thanks
master (2nd commit): 9885fc228d8b1f4cac9151d3d3afb37298b2c3cb
to uplifters, there are 2 commits on master :

c7f5c27137cc36f51c9da194292e1e5055675017
9885fc228d8b1f4cac9151d3d3afb37298b2c3cb
(In reply to Julien Wajsberg [:julienw] from comment #8)
> (In reply to Steve Chung from comment #7)
> > Comment on attachment 754541 [details] [diff] [review]
> > fix tests
> > 
> > Although it's a little bit confusing that having a " |" after the type
> > string, I think it's ok only for interpolation only.
> 
> That's what the wireframe says (maybe it's better to do this with border in
> CSS), and that was not my code ;)
> 
> thanks

Regarding the "|" in the auto suggestion. You should not be following the wireframe for this.. You should be following however Visual Design has specified the auto suggestions to be laid out. The wireframes only detail the information to be delivered and the order/hierarchy of the information being delivered. They do not detail treatment or styling.
Yep, you're right, the visual design shows the same, I checked ;) Thanks !

However I'm more concerned about accessibility, because for a screen reader, '|' is just like another character, and it will probably read it. Maybe we'll need to put it in a <span> with an aria role "presentation". Or use CSS. But this is for another time.
(In reply to Julien Wajsberg [:julienw] from comment #12)
> Yep, you're right, the visual design shows the same, I checked ;) Thanks !
> 
> However I'm more concerned about accessibility, because for a screen reader,
> '|' is just like another character, and it will probably read it. Maybe
> we'll need to put it in a <span> with an aria role "presentation". Or use
> CSS. But this is for another time.

I agree regarding screen reading. when i refer across the applications i note we actually have a different layout s for instances where the type/phonenumber line appears. sometimes they are divided by a "|" (message app) sometimes by a "," (call log) and sometimes just space.

Going to RFI Victoria to review the type/phonenumber layout again in all instances.
Flags: needinfo?(vpg)
I preffer the "|" as a separator, but taking a look at call log, and for the sake of consistency we'll go for the "," between type and phone number.

Thanks!
Flags: needinfo?(vpg)
blocking-b2g: leo? → leo+
Made new bug for separator issue https://bugzilla.mozilla.org/show_bug.cgi?id=876754
Blocks: 876754
This bug was partially uplifted.

Uplifted c7f5c27137cc36f51c9da194292e1e5055675017 to:
Uplifted 9885fc228d8b1f4cac9151d3d3afb37298b2c3cb to:
v1-train: 365475a31ca3e00f863d6fd5fb0b17accd5d98ff

Commit c7f5c27137cc36f51c9da194292e1e5055675017 didn't uplift to branch v1-train
(In reply to John Ford [:jhford] -- If you expect a reply from me, use needsinfo? instead of CC from comment #16)
> This bug was partially uplifted.
> 
> Uplifted c7f5c27137cc36f51c9da194292e1e5055675017 to:
> Uplifted 9885fc228d8b1f4cac9151d3d3afb37298b2c3cb to:
> v1-train: 365475a31ca3e00f863d6fd5fb0b17accd5d98ff
> 
> Commit c7f5c27137cc36f51c9da194292e1e5055675017 didn't uplift to branch
> v1-train

I've reverted this commit with e1c59ba
Applies properly after bug 872395
Flags: needinfo?(jhford)
Depends on: 872395
Uplifted c7f5c27137cc36f51c9da194292e1e5055675017 to:
v1-train: 0c379e27b8ed2983101bb68aacbc85c9670f0aea
Uplifted 9885fc228d8b1f4cac9151d3d3afb37298b2c3cb to:
v1-train: 0154e3133ef811ca3e1fa192b4f1be653b8a40f1
Flags: needinfo?(jhford)
The spec should be updated and re-published to reflect the change: "|" => ","
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: