1184933 - Implement the refreshed design for the failure view

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

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

 - Implement the back button, if applicable. I'm having trouble understanding the flow here, so I hope Sevaan or Vicky can clarify this.
 - The text chat input box should remain visible whenever the data channel is available and connected. I'm not sure why that would be the case and rather unhelpful during erroneous situations. I hope Sevaan or Vicky can clarify this too.
 - Implement a centered Hello glyph.
 - Implement the new text "We're having technical difficulties…"
 - Implement a button with the caption 'Rejoin Conversation'. Clicking this button will reset the state of the window entirely to the same state as when the window opened.

For the visual design spec, please check out the ones attached to bug 1179164.

Comment 1

[Victoria Gerchinhoren [:vicky]]

Sevaan should be able to answer how this behaves. So clearing NI.

Comment 2

[Sevaan Franks [:sevaan]]

>  - Implement the back button, if applicable. I'm having trouble
> understanding the flow here, so I hope Sevaan or Vicky can clarify this.

I think we only need the back button on the Edit Context screen. Clicking it returns you to the video conversation.

>  - The text chat input box should remain visible whenever the data channel
> is available and connected. I'm not sure why that would be the case and
> rather unhelpful during erroneous situations.

I'm not sure I understand what you mean here. Could you clarify?

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Sevaan Franks [:sevaan] from comment #2)
> >  - The text chat input box should remain visible whenever the data channel
> > is available and connected. I'm not sure why that would be the case and
> > rather unhelpful during erroneous situations.
> 
> I'm not sure I understand what you mean here. Could you clarify?

We just discussed this on IRC and concluded that we don't need to show the chat area in this screen.

Comment 4

[Andrei Oprea [:andreio]]

Working on the conversation list and dropping this.

Comment 5

[Sevaan Franks [:sevaan]]

Attachment: :-/ Hello Face

Comment 6

[Marina Rodríguez [:mai]]

Hi Romain, 
On the new error screen there are not space for header and message text error. What kind of error messages are relevant to show to the user?

Comment 7

[Mark Banner (:standard8)]

(In reply to Marina Rodríguez [:mai] from comment #6)
> Hi Romain, 
> On the new error screen there are not space for header and message text
> error. What kind of error messages are relevant to show to the user?

We can get error messages such as "No camera/microphone found", we could have network issues (though we don't have a specific message for those yet), and the generic one is "Something went wrong".

I would assume with the way the mock-up is presented is that we change the generic "Something went wrong" to "We're having technical difficulties...", and then replace the "We're having technical difficulties..." with a more specific message when we have one.

Sevaan: Can you confirm that's right please?

Comment 8

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184933-Implement-the-refreshed-design-for-the-f.patch

Hi,
would you mind giving me your feedback about the patch?
Best regards

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8653416 [details] [diff] [review]
0001-Bug-1184933-Implement-the-refreshed-design-for-the-f.patch

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

Deferring to Mark, because I'd like him to take a look at replacing the CallFailedView with this.
Regardless, I think you still need to update the string to display for the generic error case in loop.properties.

Comment 10

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184933-Implement-the-refreshed-design-for-the-f.patch

Missing locale changes

Comment 11

[Marina Rodríguez [:mai]]

Attachment: Captura.PNG

Hi Seevan, 
would you review the new error message screen?
Best regards

Comment 12

[Sevaan Franks [:sevaan]]

Comment on attachment 8653428 [details]
Captura.PNG

Hi Marina,

I find it's always best to do a direct side-by-side comparison of what you're building with the provided mockups as the differences are easier to spot.

See: http://i.sevaan.com/image/3J3W0q2K0u0U

- The image needs to be bigger (mockup shows a  width of 90px)
- Back button is missing.
- Settings icon is missing
- String needs updating
- Font should be #333333 and 12px (or size equivalent)
- Button colour should be #00A9DC
- Button corners should be rounded 4px
- Button font should be 12px (or size equivalent)

Comment 13

[Sevaan Franks [:sevaan]]

Attachment: Direct Calls Error

Attaching Direct Calls error screen as well.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8653427 [details] [diff] [review]
0001-Bug-1184933-Implement-the-refreshed-design-for-the-f.patch

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

I like the idea of combining the views (wanted to do it for a while), but unfortunately we can't drop all the functionality of the existing CallFailureView - the buttons there are needed for direct calls.

I would propose that we restructure the views to something like:

- FailureInfoView - containing the image & message
- CallFailureView - to replace the existing CallFailedView, uses the FailureInfoView, wraps it in a div along with the buttons.
- RoomFailureView - new view which just has the retry button in it.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +345,2 @@
>  
> +      switch (this.getStoreState().callStateReason) {

In the proposed view structure, we should just be able to pass this in as this.props.failureReason, which would make this nicer as we could have one big switch statement.

Comment 15

[Sevaan Franks [:sevaan]]

(In reply to Mark Banner (:standard8) from comment #7)
> I would assume with the way the mock-up is presented is that we change the
> generic "Something went wrong" to "We're having technical difficulties...",
> and then replace the "We're having technical difficulties..." with a more
> specific message when we have one.
> 
> Sevaan: Can you confirm that's right please?

Correct, thanks!

Comment 16

[Sevaan Franks [:sevaan]]

(In reply to Sevaan Franks [:sevaan] from comment #12)
> Comment on attachment 8653428 [details]
> Captura.PNG
> 
> Hi Marina,
> 
> I find it's always best to do a direct side-by-side comparison of what
> you're building with the provided mockups as the differences are easier to
> spot.
> 
> See: http://i.sevaan.com/image/3J3W0q2K0u0U
> 
> - The image needs to be bigger (mockup shows a  width of 90px)
> - Back button is missing.
> - Settings icon is missing
> - String needs updating
> - Font should be #333333 and 12px (or size equivalent)
> - Button colour should be #00A9DC
> - Button corners should be rounded 4px
> - Button font should be 12px (or size equivalent)

Remove the back button, actually. I don't think it's useful here.

Comment 17

[Sevaan Franks [:sevaan]]

Regarding the settings icon, the menu items can be found in the spec here: http://i.sevaan.com/image/0X2P3o1W0i3L

However, we should remove the add/edit conversation details option on this screen. So from here a user can still submit feedback or find help.

Comment 18

[Marina Rodríguez [:mai]]

Attachment: failureView.png

Hi Sevaan,
the colors of the buttons are corrects, but for some strange reason seems differents, maybe is the pc.
Would you mind reviewing again?

Comment 19

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184933-Implement-the-refreshed-design-for-the-f.patch

Comment 20

[Marina Rodríguez [:mai]]

On the current patch is pending to check the rejoin button behaviour (method handleRejoinCall (conversationViews.jsx). I'm not sure if the correct action is retrycall or open a new room and close the current window.

Best regards,
Marina

Comment 21

[Mark Banner (:standard8)]

I can take this, although Mike might want to steal it from me ;-)

Comment 22

[Sevaan Franks [:sevaan]]

Comment on attachment 8654054 [details]
failureView.png

The colour issue is due to your colour profiles on your computer, so if you say the hexcodes are correct, that's fine. You can ask David Critchley how he solved this issue.

Some things to look at (http://i.sevaan.com/image/292e2u0l201z):

- The font size for the error message should be 12. It looks larger than that. Also, is it using the correct system font?
- The gear icon size should be 10.
- The button height should be 30

Nearly there!

Comment 23

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #21)
> I can take this, although Mike might want to steal it from me ;-)

Actually, I've had an idea on how to improve our structure further. Not had chance to check how feasible it is yet, but I'll take a look on Tuesday (I'm out Mon).

Comment 24

[Mark Banner (:standard8)]

Attachment: Implement the refreshed design for the failure view

Updated for bitrot, continued work on the component structure and layouts. Possibly functionally complete, currently reviewing all the changes.

Comment 25

[Mark Banner (:standard8)]

Attachment: Failure Views

Here's the new views. The change of the toolbar from dark to light is a different bug.

The two views on the right are the error view for rooms, the one on the left is for direct calls.

Comment 26

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8656562 [details]
Failure Views

Would you please paste a screenshot? THanks.

Comment 27

[Mark Banner (:standard8)]

Attachment: Part 1. Implement the refreshed design for the failure view.

Updated patch that's now ready for review.

The basic idea here is to share the central layout of the logo and strings between the rooms & direct call views. Then make the buttons/actions part of individual views.

Comment 28

[Mark Banner (:standard8)]

Attachment: Part 2. Add the settings menu onto the failure view.

This second part adds in the settings menu to the new views. It was a little bit more work, so I thought it best to separate it out.

Comment 29

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8656562 [details]
Failure Views

Thanks Mark, only one adjustment, can you please move down image by 5px and both image + error text 10px?

That would be it.

Thanks!

Comment 30

[Mark Banner (:standard8)]

Attachment: Updated failure views

Updated screenshots for review per comments & irc discussion.

Comment 31

[Mark Banner (:standard8)]

Attachment: Part 1. Implement the refreshed design for the failure view.

Updated patches following ux changes. Note: depends on bug 1201446.

Comment 32

[Mark Banner (:standard8)]

Attachment: Part 2. Add the settings menu onto the failure view.

Comment 33

[Mark Banner (:standard8)]

Attachment: Part 1. Implement the refreshed design for the failure view. v2

Updated for Vicky's final comments from irc.

Comment 34

[Mark Banner (:standard8)]

Attachment: Part 2. Add the settings menu onto the failure view. v2

Hasn't changed but just updating to ensure no bitrot etc.

Comment 35

[Mark Banner (:standard8)]

Comment on attachment 8657735 [details] [diff] [review]
Part 1. Implement the refreshed design for the failure view. v2

Ed is going to look at these.

Comment 36

[Mark Banner (:standard8)]

Attachment: Part 1. Implement the refreshed design for the failure view. v3

I landed a couple of other patches, so this fixes the bitrot.

Comment 37

[Mark Banner (:standard8)]

Attachment: Part 2. Add the settings menu onto the failure view.

Comment 39

[Ed Lee :Mardak]

Comment on attachment 8657735 [details] [diff] [review]
Part 1. Implement the refreshed design for the failure view. v2

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

::: browser/components/loop/content/js/roomViews.jsx
@@ +97,5 @@
> +      return (
> +        <div className="room-failure">
> +          <loop.conversationViews.FailureInfoView
> +            failureReason={this.props.failureReason} />
> +          <div className="btn-group call-action-group">

I see the original CallFailedView had this div.btn-group. Any value of using loop.shared.views.ButtonGroup? There's specific .btn-group styling that might be different from .button-group, so might just clean this up later.

@@ +98,5 @@
> +        <div className="room-failure">
> +          <loop.conversationViews.FailureInfoView
> +            failureReason={this.props.failureReason} />
> +          <div className="btn-group call-action-group">
> +            <button className="btn btn-rejoin"

We can reuse .btn-info to get the styling.

::: browser/components/loop/content/shared/css/common.css
@@ +99,5 @@
>    white-space: nowrap;
>  }
>  
> +.btn-info,
> +.btn-rejoin {

No need for these btn-rejoin specific styles when reusing btn-info.

::: browser/components/loop/content/shared/css/conversation.css
@@ +344,5 @@
> +  flex-direction: column;
> +}
> +
> +.failure-info-logo {
> +  width: 100%;

This is a div, so it's width should already be full block width.

@@ +350,5 @@
> +  background-image: url("../img/sad_hello_icon_64x64.svg");
> +  background-position: center center;
> +  background-size: contain;
> +  background-repeat: no-repeat;
> +  flex: 1 1 auto;

"flex: 1" should do the same thing unless you're trying to be explicit. Or maybe just "flex-grow: 1" ?

@@ +360,5 @@
> +}
> +
> +.failure-info-message {
> +  margin: 0.25rem 0px;
> +  width: 100%;

Width 100% and no side margins could have text go all the way to the edge. We probably want some padding on these messages or on .failure-info.

@@ +370,5 @@
> +}
> +
> +.failure-info-extra,
> +.failure-info-extra-failure {
> +  margin: 0.25rem 0;

Probably want side padding here too.

::: browser/components/loop/content/shared/img/sad_hello_icon_64x64.svg
@@ +1,3 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="180px" height="173px" viewBox="0 0 180 173" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.3.3 (12081) - http://www.bohemiancoding.com/sketch -->

svgo the file

::: browser/components/loop/content/shared/js/store.js
@@ +120,4 @@
>      return {
>        getStore: function() {
> +        // Allows the ui-showcase to override the specified store.
> +        if (this.props[id]) {

props[id] could be falsy/0/false/"". Not sure if we want to fall back to store or use the props?

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +342,5 @@
> +      expect(extraFailureMessage.textContent).eql("Fake failure message");
> +    });
> +  });
> +
> +  describe("DirectCallFailureView", function() {

It doesn't look like there's a test for DirectCallFailureView with outgoing: false. We should probably make sure it has the right number of buttons.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +1087,2 @@
>                             summary="Call Failed - Incoming"
> +                           width={298}>

nit: Let's update the indent to 2 space while we're touching these. Same for other FramedExamples touched/added.

Comment 40

[Ed Lee :Mardak]

Comment on attachment 8658591 [details] [diff] [review]
Part 2. Add the settings menu onto the failure view.

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

::: browser/components/loop/content/shared/css/conversation.css
@@ +348,5 @@
> +}
> +
> +html[dir="rtl"] .direct-call-failure > .settings-control,
> +html[dir="rtl"] .room-failure > .settings-control {
> +  left: .5rem

; semicolon

::: browser/components/loop/content/shared/js/views.jsx
@@ +185,5 @@
>     */
>    var SettingsControlButton = React.createClass({
>      propTypes: {
> +      // Set to true if the menu should be below the button rather than above.
> +      menuBelow: React.PropTypes.bool,

Do we ever want the menu to be above the button? I don't see any other uses of SettingsControlButton. If not, we could simplify but perhaps later.

Comment 41

[Mark Banner (:standard8)]

(In reply to Ed Lee :Mardak from comment #39)
> Comment on attachment 8657735 [details] [diff] [review]
> ::: browser/components/loop/content/js/roomViews.jsx
> @@ +97,5 @@
> > +          <div className="btn-group call-action-group">
> 
> I see the original CallFailedView had this div.btn-group. Any value of using
> loop.shared.views.ButtonGroup? There's specific .btn-group styling that
> might be different from .button-group, so might just clean this up later.

It might be worth doing, I'll split that to a separate bug, as I think that'll take a bit of sorting out.

> @@ +360,5 @@
> > +}
> > +
> > +.failure-info-message {
> > +  margin: 0.25rem 0px;
> > +  width: 100%;
> 
> Width 100% and no side margins could have text go all the way to the edge.
> We probably want some padding on these messages or on .failure-info.

Went with 8px to match the padding/margins the buttons had, set on failure-info.

> ::: browser/components/loop/test/desktop-local/conversationViews_test.js
> @@ +342,5 @@
> > +  describe("DirectCallFailureView", function() {
> 
> It doesn't look like there's a test for DirectCallFailureView with outgoing:
> false. We should probably make sure it has the right number of buttons.

Well spotted!

Comment 42

[Mark Banner (:standard8)]

(In reply to Ed Lee :Mardak from comment #40)
> > +      // Set to true if the menu should be below the button rather than above.
> > +      menuBelow: React.PropTypes.bool,
> 
> Do we ever want the menu to be above the button? I don't see any other uses
> of SettingsControlButton. If not, we could simplify but perhaps later.

It's above the button for the conversation toolbar when you're in the conversation.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f26df0a96052
https://hg.mozilla.org/integration/fx-team/rev/65ad455cd4a0

Comment 44

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f26df0a96052
https://hg.mozilla.org/mozilla-central/rev/65ad455cd4a0

Comment 46

[Ada [:adalucinet]]

Verified that the changes made to the failure view 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. No new issues were found during testing.