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

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vicky, Assigned: rwaldron)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 872514
(Assignee)

Updated

5 years ago
Assignee: nobody → waldron.rick
(Assignee)

Comment 1

5 years ago
Created attachment 754170 [details] [review]
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+
(Assignee)

Comment 3

5 years ago
Landed https://github.com/mozilla-b2g/gaia/commit/c7f5c27137cc36f51c9da194292e1e5055675017
Status: NEW → RESOLVED
Last Resolved: 5 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)
Created attachment 754541 [details] [diff] [review]
fix tests

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)

Updated

5 years ago
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

Comment 11

5 years ago
(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.

Comment 13

5 years ago
(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)
(Reporter)

Comment 14

5 years ago
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+

Comment 15

5 years ago
Made new bug for separator issue https://bugzilla.mozilla.org/show_bug.cgi?id=876754

Updated

5 years ago
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)

Updated

5 years ago
Depends on: 872395
Uplifted c7f5c27137cc36f51c9da194292e1e5055675017 to:
v1-train: 0c379e27b8ed2983101bb68aacbc85c9670f0aea
Uplifted 9885fc228d8b1f4cac9151d3d3afb37298b2c3cb to:
v1-train: 0154e3133ef811ca3e1fa192b4f1be653b8a40f1
status-b2g18: --- → fixed
Flags: needinfo?(jhford)
(Assignee)

Comment 20

5 years ago
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.