1190442 - FTU needs UX refresh

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Screen Shot 2015-08-03 at 15.33.35.png

I'm guessing the change of the start conversation button has affected this - see the attached image (before on left, after on right).

Sevaan/Vicky: should we revert the button to the original, or is there a revised layout for this screen?

Comment 1

[Victoria Gerchinhoren [:vicky]]

(In reply to Mark Banner (:standard8) from comment #0)
> Created attachment 8642465 [details]
> Screen Shot 2015-08-03 at 15.33.35.png
> 
> I'm guessing the change of the start conversation button has affected this -
> see the attached image (before on left, after on right).
> 
> Sevaan/Vicky: should we revert the button to the original, or is there a
> revised layout for this screen?

Hi Mark,
I am working on a few screens that were not contemplated before, this is one of them, but just so you know there's a "special button" designed in the set of components that fits this case, it's exactly for cases like this where the button needs to sit alone. 

Should we change the name of this bug to "Refresh FTU?"

Thanks!

Comment 2

[Victoria Gerchinhoren [:vicky]]

Updated the mocks of the Panel UI including this FTU screen, please see user story for link.

Comment 3

[Victoria Gerchinhoren [:vicky]]

Plase, find detailed specs for implementation here: https://www.dropbox.com/s/e268m59ohd6i07n/Hello_FTU_specs.png?dl=0

Comment 4

[Andrei Oprea [:andreio]]

https://www.dropbox.com/s/qmoyj1b7fgja56u/Screenshot%202015-08-05%2021.28.21.png?dl=0

UI review.

Comment 5

[Victoria Gerchinhoren [:vicky]]

Hey Andrei, thanks, there are some things still to be refined, text in the button is bold, background color for the whole panel is a very light gray and the logo should be the whole logo, not the icon with the word, but I guess in the previous implementation there's no svg with the logo, so I am uploading one the the assets folder (root).

Remember that there's a folder where visual specs usually are, please use those as guidance: https://www.dropbox.com/s/e268m59ohd6i07n/Hello_FTU_specs.png?dl=0

But so far pretty good!

Thanks!

Comment 6

[Andrei Oprea [:andreio]]

Updated FTU Panel.

https://www.dropbox.com/home/Screenshots?preview=Screenshot+2015-08-06+19.26.31.png

Comment 7

[Andrei Oprea [:andreio]]

Fixed link.

https://www.dropbox.com/s/x71q2l8hlap5rkp/Screenshot%202015-08-06%2019.26.31.png?dl=0

Comment 8

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #7)
> Fixed link.
> 
> https://www.dropbox.com/s/x71q2l8hlap5rkp/Screenshot%202015-08-06%2019.26.31.
> png?dl=0

Looks OK. (is the background in the gray or white? should be the gray but can't tell right now)

Comment 9

[Andrei Oprea [:andreio]]

Attachment: Visual refresh for Loop FTU panel

Comment 10

[Andrei Oprea [:andreio]]

Attachment: Visual refresh for Loop FTU panel

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8646072 [details] [diff] [review]
Visual refresh for Loop FTU panel

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

In testing I found that this patch seems to stop the hover modes working on start a conversation button in the ui-showcase, the normal button seemed unaffected though, which is a bit weird.

::: browser/components/loop/content/css/panel.css
@@ +221,5 @@
> +.fte-title > img {
> +  width: 100%;
> +}
> +
> +.subheader {

Don't we want fte-subheader here? This feels a bit specific. Alternately we should move it to a location that isn't embedded within all the fte items.

@@ +230,3 @@
>  }
>  
> +.font-bold {

This is reasonable as a generic class, but we should either move it to common.css or find a better location for it.

@@ +245,5 @@
>  #share-link-header {
>    -moz-padding-start: 20px;
>  }
> +
> +.fte-get-started-container {

Looking at the ui-showcase, I think this should get a width of 100% as well.

@@ +575,5 @@
>    background-color: #ebebeb;
>    color: #c3c3c3;
>  }
>  
> +.button.button-accept:active {

I don't see the reason for this change. Wrong patch?

@@ +663,5 @@
>    border-color: #0096dd;
>    color: #fff;
>  }
>  
> +.fte-get-started-button {

It'd be nice if we could group all of the fte specific items a bit more, rather than having them spread around the whole file.

::: browser/components/loop/content/js/panel.jsx
@@ -202,5 @@
>        }
>        return (
> -        <div id="fte-getstarted">
> -          <header id="fte-title">
> -            {mozL10n.get("first_time_experience_title", {

This string isn't used anymore, so should be dropped from loop.properties.

@@ +208,5 @@
> +        <div className="fte-get-started-content">
> +          <header className="fte-title">
> +            <img src="loop/shared/img/hello_logo.svg" />
> +            <div className="subheader">
> +              {mozL10n.get("rooms_room_join_label")}

I think it would be safer for L10n to create a new string, something like:

first_time_experience_subheading

Some locales may have slightly different terminology for button labels versus sub headings, so it'll be better to have two.

::: browser/components/loop/standalone/content/css/webapp.css
@@ +114,2 @@
>    font-weight: 400;
> +  color: #4a4a4a;

The changes here should only be affecting the old call-urls which we're dropping soon. I don't think we need to worry about this at all.

::: browser/components/loop/ui/fake-l10n.js
@@ +26,5 @@
>  
>      get: function(stringId, vars) {
>  
> +      if (stringId === "first_time_experience_title") {
> +        return "Firefox Hello";

This isn't needed since the string is being dropped.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +475,5 @@
> +    if (prop === "seenToS") {
> +      return "unseen";
> +    }
> +
> +    if (prop === "showPartnerLogo") {

seenToS and showPartnerLogo were dropped as of about a week or two ago - so they can be dropped.

@@ +744,5 @@
>              <p className="note">
>                <strong>Note:</strong> 332px wide.
>              </p>
> +            <Example dashed={true}
> +                     style={{width: "332px", height: "470px"}}

Seeing as there's a bug to force the panel height to 410px, I think we could just set that height now.

It might also be worth generating a screenshot at that height and checking Vicky is still happy with the layout.

Comment 12

[Andrei Oprea [:andreio]]

(In reply to Mark Banner (:standard8) from comment #11)
> Comment on attachment 8646072 [details] [diff] [review]
> Visual refresh for Loop FTU panel
> 
> Review of attachment 8646072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +575,5 @@
> >    background-color: #ebebeb;
> >    color: #c3c3c3;
> >  }
> >  
> > +.button.button-accept:active {
> 
> I don't see the reason for this change. Wrong patch?

Doing :hover:active increases the specificity of that rule and you cannot override it.

> 
> It might also be worth generating a screenshot at that height and checking
> Vicky is still happy with the layout.

Shouldn't this be fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1192372 ?

Comment 13

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #12)
> > It might also be worth generating a screenshot at that height and checking
> > Vicky is still happy with the layout.
> 
> Shouldn't this be fixed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1192372 ?

That or one other - I've got a needinfo out on a different bug. So I'm fine punting this for now - but please make the showcase the same height as the current panel.

Comment 14

[Andrei Oprea [:andreio]]

Attachment: Visual refresh for Loop FTU panel

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8649005 [details] [diff] [review]
Visual refresh for Loop FTU panel

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

I added a screenshots of the last remaining visual glitches that you need to fix. Almost there!!

::: browser/components/loop/content/css/panel.css
@@ +962,5 @@
> +  margin: 0 15px;
> +  padding: 0;
> +  border-radius: 4px;
> +  font-size: 1.4rem;
> +  font-weight:bold;

nit: missing space.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +28,3 @@
>  first_time_experience_button_label=Get Started
> +## LOCALIZATION_NOTE(first_time_experience_subheading): Message inviting the
> +## user to create his first conversation.

nit: 'his or her'

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch review screenshot

Comment 17

[Andrei Oprea [:andreio]]

Attachment: Visual refresh for Loop FTU panel

Comment 18

[Andrei Oprea [:andreio]]

Switched to using a span instead of an image there because it was showing the default broken path image in the ui-showcase. Also makes more sense to not be an image because it is not using the `src` attribute.

Made the whitespace match the spec 34px and button height 45px.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8649567 [details] [diff] [review]
Visual refresh for Loop FTU panel

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

Looking good! With nit fixed, r=me!

::: browser/components/loop/ui/ui-showcase.jsx
@@ +736,5 @@
> +              </div>
> +            </FramedExample>
> +
> +            <FramedExample cssClass="fx-embedded-panel"
> +                           dashed={true}

nit: I see multiple styles of element attribute indentation appearing in this file, which is not how it should be. :(
Please indent the attributes with just two spaces, just like you did with `<PanelView` above.

Comment 20

[Andrei Oprea [:andreio]]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Bogdan Maris, Desktop Test Engineering]

Verified that the changes made to the FTU is the same with the intended refresh across platforms (Windows 10 32-bit, Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit) using latest Nightly 43.0a1 and 44.0a1.