1183386 - Update conversation tab headers for panel

Status: VERIFIED FIXED

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

Comment 1

[Mike de Boer [:mikedeboer]]

Sevaan, should the blue bar - which indicates which tab is currently active - animate to its new position when a different tab is selected, or should it jump into place right away?

Comment 2

[Sevaan Franks [:sevaan]]

I think it should! The blue bar should slide over.

Vicky has been putting together thoughts on animation for the visual refresh. I'm NIing her.

Comment 3

[Victoria Gerchinhoren [:vicky]]

(In reply to Sevaan Franks [:sevaan] from comment #2)
> I think it should! The blue bar should slide over.
> 
> Vicky has been putting together thoughts on animation for the visual
> refresh. I'm NIing her.

Hey, yes, please, it would be nice to have it sliding with an ease out effect when moving, and the icon turning blue as lighting up when the blue bar is almost reaching the final position. The previous active bar icon turns off as soon as the bar stars moving.

THanks!

Comment 4

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Hi,
would you mind reviewing the patch?

Best regards

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8639762 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

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

Provided feedback over IRC.

Comment 6

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Updated PR

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8639840 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

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

Apart from the comments below, there's one problem left: when you're not logged in to Firefox Accounts, the contacts tab is not visible. In that case the Conversations tab need to be centered horizontally, the blue bar _and_ the label should not be visible.

(Please see the various screens at https://www.dropbox.com/sh/46pyq5wwgnif6g8/AACEqjLwDw1be0oMYw-okHA9a?lst=&preview=PanelUI_ConversationAndContacts.png)

::: browser/components/loop/content/css/panel.css
@@ +117,5 @@
> +  pointer-events: none;
> +}
> +
> +.tab-view > li:before {
> +  content:'';

nit: missing space and please use double-quotes.

@@ +123,5 @@
> +  display: inline-block;
> +  padding-right: .2em;
> +  vertical-align: middle;
> +  height: 16px;
> +  width: 16px;

In comment 3, Vicky asked if we could turn the icon 'blue' - the '.selected' state - when the animation was _almost_ done.

I'd propose to add a `transition-property: background-image` here.
Then add a `.tab-view > li.selected` rule, which says: `transition-delay: .25s`.
...and see if that works :-P

@@ -95,3 @@
>  }
>  
> -.tab-view > li[data-tab-name="call"],

Please drop the rules for a tab named 'call'; it doesn't exist anymore.

@@ +128,5 @@
>  }
>  
> +.tab-view > li[data-tab-name="call"]:before,
> +.tab-view > li[data-tab-name="rooms"]:before {
> +    background-image: url("../shared/img/icons-16x16.svg#precall");

nit: Whoops!! Four spaces, instead of two. Please check you editors' settings.

::: browser/components/loop/content/js/panel.jsx
@@ +81,3 @@
>          }
>          var isSelected = (this.state.selectedTab == tabName);
>          if (!tab.props.hidden) {

put `var label = mozL10n.get(tabName + "_tab_button_tooltip")` here and use it below.

@@ +85,5 @@
>              <li className={cx({selected: isSelected})}
>                  data-tab-name={tabName}
>                  key={i}
>                  onClick={this.handleSelectTab}
> +                title={mozL10n.get(tabName + "_tab_button_tooltip")} >

nit: you can remove the space at the end here.

@@ +100,5 @@
>        return (
>          <div className="tab-view-container">
> +          <ul className="tab-view">
> +            {tabButtons}
> +            <div className="slide-bar"/>

nit: for auto-closing tags, our convention is to add a space between '"' and '/>'.

Comment 8

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Updated pr with your comments

Comment 9

[Marina Rodríguez [:mai]]

Attachment: refresh-tab.png

Hi Vicky,
would you mind review the patch?
Regards

Comment 10

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8640357 [details]
refresh-tab.png

PLease, can you push down the icons next to the labels by 2px or so to middle align withe the label? The "start a conversation" text is super small right now, you might be having problems with the ems...

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8640356 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

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

::: browser/components/loop/content/css/panel.css
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +html {

well, if you're adding this here... can you add `font-family: menu;` here too?

But more importantly: this makes the UI look rather horrible... I think it's warranted to _at least_ fix the few areas where the font looks problematically small.

@@ +111,4 @@
>  }
>  
> +.tab-view  li:nth-child(2):hover ~ .slide-bar {
> +  margin-left:50% !important;

The slide bar animation should happen when the tab is clicked/ selected, _not_ hovered.

@@ +126,5 @@
> +  padding-right: .2em;
> +  vertical-align: middle;
> +  height: 16px;
> +  width: 16px;
> +  transition: background-image .3s ;

I don't think you want the transition to be .3s by default (a hover effect would look very janky, for example). Better make it `transition-property: background-image;` and set the `transition-duration: .3s` when the tab is selected.

@@ +159,5 @@
> +  width: 100%;
> +}
> +
> +.tab-view li:first-child:nth-last-child(2) span {
> +  display: none;

You needn't have done this with display: flex. Why did you choose to get rid of the flexbox model here?

::: browser/components/loop/content/js/panel.jsx
@@ +89,5 @@
>                  onClick={this.handleSelectTab}
> +                title={label} >
> +              <span>
> +                {label}
> +              </span>

This'll fit on one line.

@@ +103,5 @@
>        return (
>          <div className="tab-view-container">
> +          <ul className="tab-view">
> +            {tabButtons}
> +            <div className="slide-bar"/>

nit: please add a space here. I think I mentioned that before.

Comment 12

[Marina Rodríguez [:mai]]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Comment on attachment 8640356 [details] [diff] [review]
> 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch
> 
> Review of attachment 8640356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/content/css/panel.css
> @@ +1,4 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +html {
> 
> well, if you're adding this here... can you add `font-family: menu;` here
> too?
> 
> But more importantly: this makes the UI look rather horrible... I think it's
> warranted to _at least_ fix the few areas where the font looks
> problematically small.
> 
I know, my intention is fix these problems on Bug 1183649. Currently there are a mess with the font-sizes. 
> @@ +111,4 @@
> >  }
> >  
> > +.tab-view  li:nth-child(2):hover ~ .slide-bar {
> > +  margin-left:50% !important;
> 
> The slide bar animation should happen when the tab is clicked/ selected,
> _not_ hovered.
> 
> @@ +126,5 @@
> > +  padding-right: .2em;
> > +  vertical-align: middle;
> > +  height: 16px;
> > +  width: 16px;
> > +  transition: background-image .3s ;
> 
> I don't think you want the transition to be .3s by default (a hover effect
> would look very janky, for example). Better make it `transition-property:
> background-image;` and set the `transition-duration: .3s` when the tab is
> selected.
> 
> @@ +159,5 @@
> > +  width: 100%;
> > +}
> > +
> > +.tab-view li:first-child:nth-last-child(2) span {
> > +  display: none;
> 
> You needn't have done this with display: flex. Why did you choose to get rid
> of the flexbox model here?
I cannot use display:flex because of the slide bar. The bar has to be a sibbling of the li elements. :( 

> 
> ::: browser/components/loop/content/js/panel.jsx
> @@ +89,5 @@
> >                  onClick={this.handleSelectTab}
> > +                title={label} >
> > +              <span>
> > +                {label}
> > +              </span>
> 
> This'll fit on one line.
> 
> @@ +103,5 @@
> >        return (
> >          <div className="tab-view-container">
> > +          <ul className="tab-view">
> > +            {tabButtons}
> > +            <div className="slide-bar"/>
> 
> nit: please add a space here. I think I mentioned that before.
Sorry, you're right, I suppose I made a bad rebase.

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Marina Rodríguez [:mai] from comment #12)

Ok, very clear. Go for it!

Comment 14

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Update the patch with Mike comments. Pending of change svg icon images.

Comment 15

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

updated svg files

Comment 16

[Marina Rodríguez [:mai]]

Attachment: refresh-tab1.png

Hi Vicky,
would you mind review it?
Best regards

Comment 17

[Marina Rodríguez [:mai]]

Comment on attachment 8641003 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Would you mind reviewing the patch?
Best regards

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8641003 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

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

The behavior looks good to me, except that I don't (yet) see that icon turn blue when the bar stopped animating, but it turns blue right away.
Not a direct show-stopper for me, but it'd be nice to have!

Also awaiting ui-r from Vicky...

Mark and/ or Dan can review the next iteration!

::: browser/components/loop/content/css/panel.css
@@ +119,5 @@
> +  margin-right: .2em;
> +  vertical-align: middle;
> +  height: 1.2rem;
> +  width: 1.2rem;
> +  transition-property: background-image ;

nit: superfluous space.

@@ +155,5 @@
> +.tab-view li:first-child:nth-last-child(2) {
> +  width: 100%;
> +}
> +
> +.tab-view li:first-child:nth-last-child(2) span {

Please use child-selectors - ` > span` - wherever possible.

@@ +173,5 @@
>  }
>  
> +.tab-view li:first-child:nth-last-child(3),
> +.tab-view li:first-child:nth-last-child(3) ~ li {
> +    width: 50%;

nit: indentation.

::: browser/components/loop/content/shared/img/icons-14x14.svg
@@ +47,5 @@
>    <use id="audio-active" xlink:href="#audio-shape"/>
>    <use id="audio-disabled" xlink:href="#audio-shape"/>
> +  <use id="contacts" xlink:href="#contacts-shape"/>
> +  <use id="contacts-hover" xlink:href="#contacts-shape"/>
> +  <use id="contacts-active" xlink:href="#contacts-shape"/>

Since you put the icons down here, can you remove them from the icons-16x16.svg file?

Comment 19

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Update the patch with the last comments

Comment 20

[Marina Rodríguez [:mai]]

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Comment on attachment 8641003 [details] [diff] [review]
> 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch
> 
> Review of attachment 8641003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The behavior looks good to me, except that I don't (yet) see that icon turn
> blue when the bar stopped animating, but it turns blue right away.
> Not a direct show-stopper for me, but it'd be nice to have!
> 
> Also awaiting ui-r from Vicky...
Sorry, I forget comment the problem with the animation, I'm thinking open a specific bug for this animation, because of the property background-image on firefox cannot allow animations (https://drafts.csswg.org/css-backgrounds/#background-image)
I'm studying different possibilities, but I didn't find one which I feel comfortable with this.

> Mark and/ or Dan can review the next iteration!
Ok, thanks
> ::: browser/components/loop/content/css/panel.css
> @@ +119,5 @@
> > +  margin-right: .2em;
> > +  vertical-align: middle;
> > +  height: 1.2rem;
> > +  width: 1.2rem;
> > +  transition-property: background-image ;
> 
> nit: superfluous space.
> 
> @@ +155,5 @@
> > +.tab-view li:first-child:nth-last-child(2) {
> > +  width: 100%;
> > +}
> > +
> > +.tab-view li:first-child:nth-last-child(2) span {
> 
> Please use child-selectors - ` > span` - wherever possible.
> 
> @@ +173,5 @@
> >  }
> >  
> > +.tab-view li:first-child:nth-last-child(3),
> > +.tab-view li:first-child:nth-last-child(3) ~ li {
> > +    width: 50%;
> 
> nit: indentation.
> 
> ::: browser/components/loop/content/shared/img/icons-14x14.svg
> @@ +47,5 @@
> >    <use id="audio-active" xlink:href="#audio-shape"/>
> >    <use id="audio-disabled" xlink:href="#audio-shape"/>
> > +  <use id="contacts" xlink:href="#contacts-shape"/>
> > +  <use id="contacts-hover" xlink:href="#contacts-shape"/>
> > +  <use id="contacts-active" xlink:href="#contacts-shape"/>
> 
> Since you put the icons down here, can you remove them from the
> icons-16x16.svg file?
I cannot remove the file because exists references to the old icon.

Comment 21

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8641004 [details]
refresh-tab1.png

We're reviewing and adjusting offline, so "-" to this image.

Comment 22

[Marina Rodríguez [:mai]]

Attachment: tabs.png

Updated styles

Comment 23

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Updated patch with Vicky comments

Comment 24

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8641617 [details]
tabs.png

Thanks Marina!

Comment 25

[Marina Rodríguez [:mai]]

Comment on attachment 8641619 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Hi Dan,
would you mind review the patch?
Best Regards

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 8641619 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

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

Sorry for the delay in reviewing this. In part I was concerned about the wrapping of the text if a locale has a very long name for one of the tabs - at least one locale does currently - and this would have caused the second line of text to be below the slide bar.

I wasn't sure if it was entirely possible to fix easily with the existing layout, but I've come up with a solution - I'll attach my patch in a moment, it is based on top of yours. If you could merge them together, and then fix the comments below, I think this will be ready for landing.

::: browser/components/loop/content/css/panel.css
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +html {

This section has landed elsewhere, so it can just be dropped from this patch now.

@@ +99,5 @@
> +  transition: margin .3s ease-in-out;
> +}
> +
> +.tab-view li:nth-child(1).selected ~ .slide-bar {
> +  margin-left: 0;

These two rules need RTL versions. You can see the issues in the ui-showcase, if you start up the standalone server, and then load http://localhost:3000/ui/?rtl=1#PanelView (there's a checkbox for selecting between rtl or not).

Basically, you need to prefix the selectors of the new copies of the rule with html[dir="rtl"], and then do the opposite for rtl mode, i.e. margin-left: 50% for this first rule, and 0 for the second.

@@ +115,5 @@
> +.tab-view > li:before {
> +  content: "";
> +  pointer-events: none;
> +  display: inline-block;
> +  margin-right: .5rem;

As we're in the panel and specifically, firefox only css, we can use -moz-margin-end rather than margin-right, and that'll deal with RTL mode for us.

::: browser/components/loop/content/js/panel.jsx
@@ +86,5 @@
>              <li className={cx({selected: isSelected})}
>                  data-tab-name={tabName}
>                  key={i}
>                  onClick={this.handleSelectTab}
> +                title={label}>

I think we can just drop the tooltip now - there's no point when they are the same.

Given that, could you also rename the strings to drop the "_tooltip" please? This will make it clearer to L10n that these are now button labels rather than tooltips.

::: browser/components/loop/content/shared/img/icons-14x14.svg
@@ +56,5 @@
>    <use id="hangup-active" xlink:href="#hangup-shape"/>
>    <use id="hangup-disabled" xlink:href="#hangup-shape"/>
> +  <use id="hello" xlink:href="#hello-shape"/>
> +  <use id="hello-hover" xlink:href="#hello-shape"/>
> +  <use id="hello-active" xlink:href="#hello-shape"/>

Please can you add the contacts* and hello* ids to SVGIcons#shapes in ui-showcase.jsx please.

::: browser/components/loop/content/shared/img/icons-16x16.svg
@@ -112,5 @@
>    <use id="history-active" xlink:href="#history-shape"/>
>    <use id="leave" xlink:href="#leave-shape"/>
> -  <use id="precall" xlink:href="#precall-shape"/>
> -  <use id="precall-hover" xlink:href="#precall-shape"/>
> -  <use id="precall-active" xlink:href="#precall-shape"/>

Please can you remove the precall* references from the SVGIcons#shapes section in ui-showcase.jsx.

Comment 27

[Mark Banner (:standard8)]

Attachment: Panel layout adjustment

Here's my adjustments, I changed from a span to a div as that seemed to give slightly more flexibility, though I'm not sure it was entirely necessary.

The ui-showcase gives longer strings for these buttons, so you can check the layout there.

Please merge these together into one patch for the final review.

Comment 28

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

Updated and rebased, would you mind reviewing again?
Best regards

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8643966 [details] [diff] [review]
0001-Bug-1183386-Update-conversation-tab-headers-for-pane.patch

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

That's better, looks good. r=Standard8

I'll land this for you now since there's no further changes needed & it is in my tree anyway.

Comment 30

[Pulsebot]

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

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/80f658adde86

Comment 32

[Bogdan Maris, Desktop Test Engineering]

Did some exploratory testing on latest Nightly 42.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit) and the implementation looks good, only found one issue that was added to the dependencies. Marking this bug as verified fixed.

Comment 33

[Mark Banner (:standard8)]

Attachment: Backout for Aurora (42)

Approval Request Comment
[Feature/regressing bug #]: FF Hello Visual Refresh, backout patch 1 of 5
[User impact if declined]: Partial refresh implementation landed for 42, but is now being completed for 43 and isn't suitable for uplift. Hence, the current partially done 42 has bad layouts and an incomplete design change.
[Describe test coverage new/current, TreeHerder]: Code has unit tests
[Risks and why]: Low - reverting to the previous version.
[String/UUID change made/needed]: None. Any string changes were not backed out.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8658612 [details] [diff] [review]
Backout for Aurora (42)

managed in bug 1203454