1190738 - Add Edit Contact view with error to ui-showcase

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Bogdan Maris, Desktop QA]

Attachment: Screenshot showing the issue

Affected builds:
- latest Nighty 42.0a1

Affected OS`s:
- Windows 7 64-bit
- Ubuntu 14.04 32-bit
- Mac OS X 10.10

STR:
1. Start Firefox
2. Click Hello icon
3. Click Get started
4. Visit a webpage (eg: https://www.mozilla.org/en-US/firefox/hello/)
5. Tick 'Let`s talk about' checkbox
6. Start a conversation
7. Edit context

Expected results: Tickbox has a propper size way to large in comparison with the strings 

Actual results: Tickbox way to large.

Notes:
- This is a recent regression:

m-c
Last good revision: ca53d4297f02
First bad revision: aeb85029c3b3
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ca53d4297f02&tochange=aeb85029c3b3

m-i
Last good revision: 104b0bbd714f
First bad revision: 57273aac7996
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=104b0bbd714f&tochange=57273aac7996

I don`t know if this is right but only this bug looks somewhat related:
 - 35238dc29bd8	 Marina Rodriguez Iglesias — Bug 1183649 - Implement the refreshed design for the 'Start a new conversation' button. r=mikedeboer

Comment 1

[Mark Banner (:standard8)]

Possibly will get fixed by bug 1191392.

Comment 2

[:shell escalante]

just want to make sure we don't fix with visual refresh before taking higher.  leaving open to verify after the bugs land

Comment 3

[David Critchley (:dcritch)]

Combine checkbox wrapper in panel.css with room-context .checkbox-wrapper in conversation.css and place in .checkbox-wrapper in common.css (line-height 2.2rem and margin-bottom 0.5rem)

Width 100% to go in checkbox-wrapper in panel.

padding-top 1px for text
.checkbox in common.css change background-size: 2rem;
    - Change EM to REM sizes

Comment 4

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1190738 - Updating checkbox in conversation/panel window

Merging Loop CSS for checkboxes

Comment 5

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1190738 - Updating checkbox in conversation/panel window

Merging Loop CSS for checkboxes

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8659407 [details] [diff] [review]
Attachment to Bug 1190738 - Updating checkbox in conversation/panel window

Review of attachment 8659407 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Please add a ui-showcase view for the context so that it's more easily discoverable, and so that the screenshot tool captures changes to that view.

Comment 7

[Mark Banner (:standard8)]

David, please can we get this updated/reviewed today?

Comment 8

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1190738 - Updating checkbox in conversation/panel window

Merging Loop CSS for checkboxes

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8662681 [details] [diff] [review]
Attachment to Bug 1190738 - Updating checkbox in conversation/panel window

Review of attachment 8662681 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; r=dmose.

Comment 10

[Ed Lee :Mardak]

The edit context view in the updated visuals (bottom-right view) https://www.dropbox.com/sh/46pyq5wwgnif6g8/AACEqjLwDw1be0oMYw-okHA9a?dl=0&preview=ConversationWindow.png seems to not have a checkbox. Are there some views that will have the checkbox and others not?

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

No, the plan is to just have your patch remove that checkbox.  The main point of landing this is for the ui-showcase bits.  

I'll rebase and update your patch after landing this.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Merging Loop CSS for checkboxes

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

(And yes, I get that this is a bit goofy, but with the way things are in-flight, I think it's actually the simplest way to go).

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Merging Loop CSS for checkboxes

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/18f5bcaab8ec

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #11)
> No, the plan is to just have your patch remove that checkbox.  The main
> point of landing this is for the ui-showcase bits.  
> 
> I'll rebase and update your patch after landing this.

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #13)
> (And yes, I get that this is a bit goofy, but with the way things are
> in-flight, I think it's actually the simplest way to go).

As it turned out, this was a bad idea.  I've backed out this patch (sorry for the bad advice, Dave!).

Dave, can you generate a new version of the patch with only the ui-showcase changes?

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/a97f5b00b772

Comment 18

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1190738 - Updating checkbox in conversation/panel window

Adding UI-Showcase test for Edit Context view

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d6bc82486876

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8665711 [details] [diff] [review]
Attachment to Bug 1190738 - Updating checkbox in conversation/panel window

Review of attachment 8665711 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; r=dmose

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6bc82486876

Comment 22

[Bogdan Maris, Desktop QA]

Verified that the checkbox was removed and the new design was correct implemented on Firefox 44 beta 2, across platforms (Windows 7 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 32-bit).