Closed Bug 1112539 Opened 5 years ago Closed 5 years ago

[Messages] Number-only contact details are broken in remove recipient confirmation dialog

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: azasypkin, Assigned: azasypkin)

References

Details

(Whiteboard: [sms-sprint-2.2S3])

Attachments

(3 files)

STR:

* Open Messages app and navigate to Composer/New Message panel;
* Focus on recipients input and pick existing contact that doesn't have phone type and carrier defined;
* Tap on selected recipient pill and observe confirmation dialog.

Expected result: 
* Dialog should show phone number only in contact details section;

Actual result:
* Contact details section consists of two lines, first line contains "," only, second line contains actual phone number.

Not sure if it's possible to have contact with phone number without phone type defined in real, but I see it a lot for contacts from workload data that's annoying :)
Comment on attachment 8537781 [details] [review]
GitHub pull request URL

Hey Julien,

Here is not so nice BB rule override that solves the problem, but don't have any better idea for now other then separate webcomponent for contact details.

Could you please take a look?

Thanks!
Attachment #8537781 - Flags: review?(felash)
Comment on attachment 8537781 [details] [review]
GitHub pull request URL


I'd prefer to get rid of the bad selector in the BB ;) If it's used somewhere we should rather move it to the specific app, possibly using a class, than have such a generic rule applied globally.

I see that Arnau is away until 9th January, so maybe you'll want to look at this yourself until he comes back?
Attachment #8537781 - Flags: review?(felash)
+ one more usecase to verify from bug 1114046 - carrier-less contacts when carrier separator isn't whitespace, eg. comma
Duplicate of this bug: 1114046
Attachment #8542087 - Flags: ui-review?(jelee)
https://github.com/mozilla-b2g/gaia/blob/master/shared/style/confirm.css#L170

Hey Jenny,

In this patch I'm going to change some BB shared style that will affect one of the settings dialogs and :EragonJ pointed that you can help with settings stuff too :)

So in attachment 8542083 [details] you can see current style and in attachment 8542087 [details] what happens if my patch is applied.

I just want to confirm if I need to preserve current style because? As it's kind of weird IMO :)

Thanks!
Whiteboard: [sms-sprint-2.2S3]
Comment on attachment 8542087 [details]
New call waiting dialog (Settings)

Looks good!
Attachment #8542087 - Flags: ui-review?(jelee) → ui-review+
Comment on attachment 8537781 [details] [review]
GitHub pull request URL

(In reply to Jenny Lee from comment #9)
> Comment on attachment 8542087 [details]
> New call waiting dialog (Settings)
> 
> Looks good!

Thanks Jenny!

Hey Julien and Arnau,

Here's the patch that updates confirm dialog BB style from "p span" to "p > span". Ideally it should not change anything for dialogs that use "p->span" markup inside as for almost all usages I've found [1], "span" is direct child of "p". Exception from that rule is confirmed in comment 9.

Thanks! 

[1] https://github.com/mozilla-b2g/gaia/pull/26846#discussion_r22243571
Attachment #8537781 - Flags: review?(rnowmrch)
Attachment #8537781 - Flags: review?(felash)
Comment on attachment 8537781 [details] [review]
GitHub pull request URL

r=me with one optional nit, good cleanup !
Attachment #8537781 - Flags: review?(felash) → review+
Comment on attachment 8537781 [details] [review]
GitHub pull request URL

LGTM, thanks Oleg.
Sorry for the late review, I was on PTO.
Please ping Pavel for future BB reviews ;)
Attachment #8537781 - Flags: review?(rnowmrch) → review+
No worries, thanks a lot, Arnau!

Treeherder is green, so landed.

Master: https://github.com/mozilla-b2g/gaia/commit/c1c7403fab9d59c6d400ec371499ff8a277bb141
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.