Update conversation tab headers for panel

VERIFIED FIXED in Firefox 42

Status

defect
P1
normal
Rank:
17
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: dmose, Assigned: mai)

Tracking

unspecified
mozilla42
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox42 verified)

Details

(Whiteboard: [visual refresh])

User Story

As a user, when I drop down the Hello panel, I see tab headers that contain both a title and an icon, and match the rest of the refreshed UI.

Acceptance criteria:

* uses updated icon assets
* no divider between the two tabs
* thin horizontal blue bar below selected tab
* matches mockups in bug 1179193 as well 
* may or may not keep beta ribbon (see bug 1179193 comment 2)

Panel UI Conversation & Contacts mockup: http://i.sevaan.com/image/0y350t1Y1230
Common elements mockup: http://i.sevaan.com/image/150W352s203b


This is part of the breakdown for bug 1179193.

Attachments

(4 attachments, 9 obsolete attachments)

9.38 KB, image/png
vicky
: ui-review+
Details
2.99 KB, patch
Details | Diff | Splinter Review
27.84 KB, patch
standard8
: review+
Details | Diff | Splinter Review
26.73 KB, patch
Details | Diff | Splinter Review
No description provided.
Flags: qe-verify+
Flags: firefox-backlog+
Points: --- → 3
Priority: -- → P1
Whiteboard: [visual refresh]
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?
Flags: needinfo?(sfranks)
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.
Flags: needinfo?(sfranks) → needinfo?(vpg)
(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!
Flags: needinfo?(vpg)
Assignee: nobody → marina.rodrigueziglesias
Iteration: --- → 42.2 - Jul 27
Rank: 19
Rank: 19 → 17
Hi,
would you mind reviewing the patch?

Best regards
Attachment #8639762 - Flags: review?(mdeboer)
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.
Attachment #8639762 - Flags: review?(mdeboer) → feedback+
Updated PR
Attachment #8639762 - Attachment is obsolete: true
Attachment #8639840 - Flags: review?(mdeboer)
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 '/>'.
Attachment #8639840 - Flags: review?(mdeboer) → feedback+
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
Updated pr with your comments
Attachment #8639840 - Attachment is obsolete: true
Attachment #8640356 - Flags: review?(mdeboer)
Posted image refresh-tab.png (obsolete) —
Hi Vicky,
would you mind review the patch?
Regards
Attachment #8640357 - Flags: ui-review?(vpg)
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...
Attachment #8640357 - Flags: ui-review?(vpg) → ui-review-
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.
Attachment #8640356 - Flags: review?(mdeboer) → feedback+
(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.
(In reply to Marina Rodríguez [:mai] from comment #12)

Ok, very clear. Go for it!
Update the patch with Mike comments. Pending of change svg icon images.
Attachment #8640356 - Attachment is obsolete: true
updated svg files
Attachment #8640561 - Attachment is obsolete: true
Posted image refresh-tab1.png (obsolete) —
Hi Vicky,
would you mind review it?
Best regards
Attachment #8640357 - Attachment is obsolete: true
Attachment #8641004 - Flags: ui-review?(vpg)
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
Attachment #8641003 - Flags: review?(mdeboer)
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?
Attachment #8641003 - Flags: review?(mdeboer) → review-
Update the patch with the last comments
Attachment #8641003 - Attachment is obsolete: true
(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 on attachment 8641004 [details]
refresh-tab1.png

We're reviewing and adjusting offline, so "-" to this image.
Attachment #8641004 - Flags: ui-review?(vpg) → ui-review-
Posted image tabs.png
Updated styles
Attachment #8641004 - Attachment is obsolete: true
Attachment #8641617 - Flags: ui-review?(vpg)
Updated patch with Vicky comments
Attachment #8641552 - Attachment is obsolete: true
Comment on attachment 8641617 [details]
tabs.png

Thanks Marina!
Attachment #8641617 - Flags: ui-review?(vpg) → ui-review+
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
Attachment #8641619 - Flags: review?(dmose)
Attachment #8641619 - Flags: review?(standard8)
Attachment #8641619 - Flags: review?(dmose)
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.
Attachment #8641619 - Flags: review?(standard8) → review-
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.
Updated and rebased, would you mind reviewing again?
Best regards
Attachment #8641619 - Attachment is obsolete: true
Attachment #8643966 - Flags: review?(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.
Attachment #8643966 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/80f658adde86
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.
Attachment #8658612 - Flags: approval-mozilla-aurora?
Attachment #8658612 - Attachment is patch: true
Duplicate of this bug: 1084279
Duplicate of this bug: 1084274
Blocks: 1203454
Comment on attachment 8658612 [details] [diff] [review]
Backout for Aurora (42)

managed in bug 1203454
Attachment #8658612 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.