Implement the refreshed design for the conversation/ chatbox titlebar

VERIFIED FIXED in Firefox 43

Status

P1
normal
Rank:
17
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

unspecified
mozilla44
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox43 verified, firefox44 verified)

Details

(Whiteboard: [visual refresh])

Attachments

(4 attachments, 6 obsolete attachments)

1.20 KB, patch
standard8
: review+
Details | Diff | Splinter Review
1.47 KB, image/svg+xml
Details
343.21 KB, image/png
sevaan
: ui-review+
Details
96.38 KB, patch
standard8
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
To meet the acceptance criteria as stated in bug 1179164, we should:

 - re-instate the light design of the chatbox, thus removing the dark theme we introduced. The old theme might not be good enough yet, so please make adjustments to it as necessary.
 - Replace the 'Close' button (x) with a 'Hang Up' button. Note that this button will be removed from the conversation toolbar and will only appear here.
 - The close button should _not_ close the conversation window right away, but instead behave just like the current 'Hang Up' button does - it might show the feedback request after the call! Therefore it's important that a button press sends an event to the content window to request to leave the conversation and close the window when appropriate.

For the visual design spec, please check out the ones attached to bug 1179164.
Flags: qe-verify+
Flags: firefox-backlog+

Updated

3 years ago
Rank: 19

Updated

3 years ago
Rank: 19 → 17
(Assignee)

Updated

3 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 43.1 - Aug 24
(Assignee)

Comment 1

3 years ago
Created attachment 8650383 [details] [diff] [review]
Patch 1: remove the deprecated dark chatbox theme

I folded the socialchat.xml change into Patch 2, because I'm touching the same function twice.
Attachment #8650383 - Flags: review?(mixedpuppy)
(Assignee)

Comment 2

3 years ago
Created attachment 8650385 [details] [diff] [review]
Patch 2: custom buttons in titlebar and chatbox theme refresh
Attachment #8650385 - Flags: review?(mixedpuppy)
(Assignee)

Updated

3 years ago
Attachment #8650385 - Flags: review?(andrei.br92)
(Assignee)

Comment 3

3 years ago
I need a new SVG version for the pop-out icon as shown here: https://www.dropbox.com/sh/46pyq5wwgnif6g8/AACEqjLwDw1be0oMYw-okHA9a?dl=0&preview=Share_Screen.png

Sevaan, can you or Michael provide it? Thanks!
Flags: needinfo?(sfranks)
Mike, would you be able to create an asset? http://i.sevaan.com/image/0m0F0k2V1s2j
Flags: needinfo?(sfranks) → needinfo?(mmaslaney)
Comment on attachment 8650385 [details] [diff] [review]
Patch 2: custom buttons in titlebar and chatbox theme refresh

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

I am not familiar with some of the codebase this touches so most of my review comments are questions :)
I expected to see some tests for the code that was added in.

::: browser/modules/Chat.jsm
@@ +252,5 @@
> +    let document = chatbox.ownerDocument;
> +    let titlebarNode = document.getAnonymousElementByAttribute(chatbox, "class",
> +      "chat-titlebar");
> +    let buttonsSeen = new Set();
> +    

Whitespace.

@@ +268,5 @@
> +        let button = gCustomButtons.get(buttonId);
> +        let buttonClass = "chat-" + buttonId;
> +        // Custom buttons are not defined in the chatbox binding, thus not
> +        // anonymous elements.
> +        node = titlebarNode.querySelector("." + buttonClass);

Could we use getElementsByClassName since it will always be a CSS class that we query for.

@@ +290,5 @@
> +        }
> +
> +        // When the chat is undocked and the button wants to be visible then, it
> +        // will be.
> +        node.hidden = isUndocked && !button.visibleWhenUndocked ? true : false;

Couldn't this just be `node.hidden = isUndocked && !button.visibleWhenUndocked;`

@@ +299,5 @@
> +
> +    // Hide any button that is part of the default set, but not of the current set.
> +    for (let button of kDefaultButtonSet) {
> +      if (buttonsSeen.has(button))
> +        continue;

Could this be `if (!buttonsSeen.has(button))` and skip the continue statement?

::: browser/themes/shared/social/chat.inc.css
@@ +238,5 @@
>    padding: 0px;
> +  border-top-left-radius: 8px;
> +  border-top-right-radius: 8px;
> +  border-bottom-left-radius: 8px;
> +  border-bottom-right-radius: 8px;

Could this be `border-radius: 8px;`?
Attachment #8650385 - Flags: review?(andrei.br92) → review+

Updated

3 years ago
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7
(Assignee)

Comment 6

3 years ago
Created attachment 8655435 [details] [diff] [review]
Patch 2.1: custom buttons in titlebar and chatbox theme refresh

Now with Andrei's review comments addressed and an integration test.
Attachment #8650385 - Attachment is obsolete: true
Attachment #8650385 - Flags: review?(mixedpuppy)
Attachment #8655435 - Flags: review?(standard8)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8650383 [details] [diff] [review]
Patch 1: remove the deprecated dark chatbox theme

Deferring review to Mark...
Attachment #8650383 - Flags: review?(mixedpuppy) → review?(standard8)
Created attachment 8657105 [details]
Screen shot of current patches

Vicky/Sevaan: There's an issue here - here's the conversation window overlaying the start page. I think the title bar colours are a little bit too close to make this scheme sensible...

Sevaan, we're also still waiting on the image from Michael as well.
Attachment #8657105 - Flags: ui-review?(vpg)
Attachment #8657105 - Flags: ui-review?(sfranks)
There should be a dropshadow, which will help separate it. Vicky, what do you think?
Attachment #8657105 - Flags: ui-review?(sfranks) → ui-review-
Comment on attachment 8657105 [details]
Screen shot of current patches

Oh this looks so much better! Only thing is to update the exit icon as it has changed to match the style of others in the conversation window. You can find it with the conversation icons sprite in the assets on Dropbox.

THanks!
Attachment #8657105 - Flags: ui-review?(vpg) → ui-review-

Updated

3 years ago
Assignee: mdeboer → standard8
Iteration: 43.2 - Sep 7 → 43.3 - Sep 21
Handing back to Mike...
Assignee: standard8 → mdeboer
Attachment #8650383 - Flags: review?(standard8) → review+
Comment on attachment 8655435 [details] [diff] [review]
Patch 2.1: custom buttons in titlebar and chatbox theme refresh

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

Ok, generally looks good, but this needs update for the UX comments, and for the comments I added below.

::: browser/components/loop/modules/MozLoopService.jsm
@@ +87,5 @@
>  
>  // See LOG_LEVELS in Console.jsm. Common examples: "All", "Info", "Warn", & "Error".
>  const PREF_LOG_LEVEL = "loop.debug.loglevel";
>  
> +const kChatboxCloseButton = {

I think it might be better to call this kChatboxHangupButton - the id of it is hangup, so its a bit strange later on when you register a 'close' but then make visible the 'loop-hangup' button.

@@ +1014,3 @@
>        chatboxInstance.setAttribute("customSize", "loopDefault");
>        chatboxInstance.parentNode.setAttribute("customSize", "loopDefault");
> +      Chat.loadButtonSet(chatboxInstance, "minimize,swap,loop-hangup");

How about "minimize,swap," + kChatboxCloseButton.id to help future maintainability? (or maybe a separate constant defined near kChatboxCloseButton).

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +81,5 @@
>    });
>  
>    describe("#init", function() {
>      var OTRestore;
> +

Unnecessary change? or maybe a missing LoopHangupNow test?

::: browser/modules/Chat.jsm
@@ +212,5 @@
>    },
> +
> +  /**
> +   * Adds a button to the collection of custom buttons that can be added to the
> +   * titlebar of a chatbox.

I think it would be useful to reference that for the button to be visible, loadButtonSet has to be called with the new button's id in the buttonSet parameter. i.e. just a way to tie these two functions together through documentation.

::: browser/themes/shared/social/chat.inc.css
@@ +123,5 @@
>    text-shadow: none;
>    cursor: inherit;
>  }
>  
>  .chat-titlebar {

Presumably we're happy that the changes we're doing here will affect social API as well?
Attachment #8655435 - Flags: review?(standard8)
Created attachment 8664393 [details]
Pop Out Icon (SVG)

(In reply to Sevaan Franks [:sevaan] from comment #4)
> Mike, would you be able to create an asset?
> http://i.sevaan.com/image/0m0F0k2V1s2j

Icon attached from :mmaslaney
Flags: needinfo?(mmaslaney)
(Assignee)

Comment 14

3 years ago
(In reply to Mark Banner (:standard8) from comment #12)
> Presumably we're happy that the changes we're doing here will affect social
> API as well?

Yes, since we're basically the only consumer of this part of the API. Apart from that fact, consider this a small visual refresh of the chat window styles as well and it's good that all consumers, including Hello, look the same.
(Assignee)

Comment 15

3 years ago
Created attachment 8665953 [details]
Screen shot of current patches

If I need to replace the icon of the hangup button, please let me know where I can find it - I don't know which URL(s) to check anymore :-/
Attachment #8657105 - Attachment is obsolete: true
Attachment #8665953 - Flags: ui-review?(sfranks)
(Assignee)

Comment 16

3 years ago
Oh and a question! What should the hover and active (click) states look like of the camera, minimize and expand buttons?
(Assignee)

Updated

3 years ago
Iteration: 43.3 - Sep 21 → 44.1 - Oct 5
Comment on attachment 8665953 [details]
Screen shot of current patches

Let's move the icons left of the red area over, so the spacing between the popout icon and the red area is the same as the spacing between the minimize and popout icon.

http://i.sevaan.com/image/1t390t1C0m09

In this mockup I moved the icons over 22px.

+r with that change.
Attachment #8665953 - Flags: ui-review?(sfranks) → ui-review+
(In reply to Mike de Boer [:mikedeboer] from comment #16)
> Oh and a question! What should the hover and active (click) states look like
> of the camera, minimize and expand buttons?

Just colour changes:

Camera Icon

- Hover: #EB6B30
- Active: #EB6B30

Minimize + Pop-out Icons

- Hover: #4a4a4a
- Active: #4a4a4a

Leave Conversation (Icon remains white, and red background changes)

- Hover: #EF6745
- Active: #EF6745
(Assignee)

Comment 19

3 years ago
(In reply to Sevaan Franks [:sevaan] from comment #18)
> Just colour changes:
> 
> Camera Icon
> 
> - Hover: #EB6B30
> - Active: #EB6B30
> 
> Minimize + Pop-out Icons
> 
> - Hover: #4a4a4a
> - Active: #4a4a4a
> 
> Leave Conversation (Icon remains white, and red background changes)
> 
> - Hover: #EF6745
> - Active: #EF6745

Sorry, I'd really like a quick mockup, because I also need to know how much of the titlebar needs to be covered by the button. Also, #4a4a4a is quite dark; should the icons change color as well?
Flags: needinfo?(sfranks)
> Sorry, I'd really like a quick mockup, because I also need to know how much
> of the titlebar needs to be covered by the button. Also, #4a4a4a is quite
> dark; should the icons change color as well?


No button. Only changing the colours of the elements.

http://i.sevaan.com/image/0s3o1D2y470s
Flags: needinfo?(sfranks)
(Assignee)

Comment 21

3 years ago
Created attachment 8666688 [details] [diff] [review]
Patch 2.2: custom buttons in titlebar and chatbox theme refresh
Attachment #8655435 - Attachment is obsolete: true
Attachment #8666688 - Flags: review?(standard8)
Comment on attachment 8666688 [details] [diff] [review]
Patch 2.2: custom buttons in titlebar and chatbox theme refresh

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

Per irc, I think we're missing the hangup button when the window is popped-out.

::: browser/components/loop/content/shared/js/views.jsx
@@ +458,5 @@
>        });
>        return (
>          <ul className={conversationToolbarCssClasses}>
> +          {
> +            this.props.showHangup ? 

eslint says:

content/shared/js/views.jsx
  499:36  error  Trailing spaces not allowed  no-trailing-spaces
Attachment #8666688 - Flags: review?(standard8)
(Assignee)

Updated

3 years ago
Blocks: 1201308
(Assignee)

Comment 23

3 years ago
Created attachment 8667854 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

This patch now abstracts the window object event listeners to the conversationAppStore, so that all event listeners can be unit tested efficiently.
Attachment #8666688 - Attachment is obsolete: true
Attachment #8667854 - Flags: review?(standard8)
(Assignee)

Updated

3 years ago
Attachment #8667854 - Attachment is obsolete: true
Attachment #8667854 - Flags: review?(standard8)
(Assignee)

Comment 24

3 years ago
Created attachment 8667856 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh
Attachment #8667856 - Flags: review?(standard8)
(Assignee)

Comment 25

3 years ago
Comment on attachment 8667856 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

Raaaah! My queues are messed up! Sorry for the noise, ppl :(
Attachment #8667856 - Flags: review?(standard8)
(Assignee)

Comment 26

3 years ago
Created attachment 8667877 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh
Attachment #8667856 - Attachment is obsolete: true
Attachment #8667877 - Flags: review?(standard8)
Comment on attachment 8667877 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

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

Looks really nice. Moving the window listeners was a good idea as well.
Attachment #8667877 - Flags: review?(standard8) → review+
(Assignee)

Comment 28

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/8e4a248e5e2da2faeb06526fe4e29006e6a5d477
Bug 1184921: remove the deprecated dark chatbox theme. r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/d44a52b0faef48ef6d38df62eda63e78d391a7e7
Bug 1184921: allow custom buttons to be added to the chatbox titlebar and implement one for Hello that closes the window when clicked. r=Standard8
https://hg.mozilla.org/mozilla-central/rev/8e4a248e5e2d
https://hg.mozilla.org/mozilla-central/rev/d44a52b0faef
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8650383 [details] [diff] [review]
Patch 1: remove the deprecated dark chatbox theme

Approval Request Comment
[Feature/regressing bug #]: Hello Visual refresh - final parts 
[User impact if declined]: The conversation window title bar will be inconsistent with the rest of the Hello styling in 43.
[Describe test coverage new/current, TreeHerder]: Combined patches have tests for the Hello code and the social changes.
[Risks and why]: There's some changes to the social conversation window, however Hello is the only ones using it. The other changes are lower risk to Hello.
[String/UUID change made/needed]: None

Note: Both patches on this bug need to be taken together. We'll also need the patches on the dependent bugs (bug 1210513, bug 1210707, bug 1211219) which were minor follow ups as we'd missed some of the control flow, but we've got it all right now.
Attachment #8650383 - Flags: approval-mozilla-aurora?
Comment on attachment 8667877 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

Approval Request Comment

See comment 30.
Attachment #8667877 - Flags: approval-mozilla-aurora?
status-firefox43: --- → affected
Comment on attachment 8650383 [details] [diff] [review]
Patch 1: remove the deprecated dark chatbox theme

Several patches to uplift to aurora, including tests.
Attachment #8650383 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8667877 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

This needs uplift to aurora for Hello theme changes.
Attachment #8667877 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: bogdan.maris
I tested the new refreshed design on Firefox Developer Edition 43.0a2 and latest Nightly 44.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit), no major issues found except bug 1218384 which will be treated separately.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
status-firefox44: fixed → verified
Flags: qe-verify+
Depends on: 1232659
You need to log in before you can comment on or make changes to this bug.