Add Edit Contact view with error to ui-showcase

VERIFIED FIXED in Firefox 44

Status

defect
P2
normal
Rank:
23
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: bogdan_maris, Assigned: dcritchley)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox44 verified)

Details

(Whiteboard: [context][visual refresh])

Attachments

(2 attachments, 5 obsolete attachments)

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
Blocks: 1179193
Summary: Large checkbox in conversation window → Large checkbox in conversation window (in comparison to text size)
Possibly will get fixed by bug 1191392.
Depends on: 1191392
just want to make sure we don't fix with visual refresh before taking higher.  leaving open to verify after the bugs land
Rank: 23
Priority: -- → P2
Whiteboard: [context] → [context][visual refersh]
Whiteboard: [context][visual refersh] → [context][visual refresh]
Assignee: nobody → david.critchley
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
Summary: Large checkbox in conversation window (in comparison to text size) → Updating checkbox in conversation/panel window
Merging Loop CSS for checkboxes
Attachment #8658957 - Flags: review?(dmose)
Attachment #8658957 - Attachment is obsolete: true
Attachment #8658957 - Flags: review?(dmose)
Merging Loop CSS for checkboxes
Attachment #8659407 - Flags: review?(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.
Attachment #8659407 - Flags: review?(dmose) → feedback+
David, please can we get this updated/reviewed today?
Flags: needinfo?(david.critchley)
Merging Loop CSS for checkboxes
Attachment #8659407 - Attachment is obsolete: true
Attachment #8662681 - Flags: review?(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.
Attachment #8662681 - Flags: review?(dmose) → review+
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?
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.
Attachment #8662681 - Attachment is obsolete: true
Attachment #8662746 - Flags: review+
(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).
Attachment #8662746 - Attachment is obsolete: true
Attachment #8662747 - Flags: review+
Flags: needinfo?(david.critchley)
Attachment #8662747 - Flags: review+ → review-
(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?
Adding UI-Showcase test for Edit Context view
Attachment #8662747 - Attachment is obsolete: true
Attachment #8665711 - Flags: review?(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
Attachment #8665711 - Flags: review?(dmose) → review+
Summary: Updating checkbox in conversation/panel window → Add Edit Contact view with error to ui-showcase
https://hg.mozilla.org/mozilla-central/rev/d6bc82486876
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.1 - Oct 5
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.