1184940 - Implement the refreshed design for the edit context 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, which the replaces the 'Close' (or 'Cancel', 'x') button, but keeps its functionality.
 - Rename the 'Save' button to 'Done'.
 - Add/ implement a 'Cancel' button, which does the exact same thing as the back button.
 - The context view should remain visible below the two 'Done' and 'Cancel' button, which will behave as a preview area that updates when you change a value in the form.

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

Comment 1

[Ed Lee :Mardak]

sevaan, do we have the back icon used in the bottom row of https://www.dropbox.com/sh/46pyq5wwgnif6g8/AACEqjLwDw1be0oMYw-okHA9a?dl=0&preview=ConversationWindow.png in the top left corner of each of those views?

Comment 2

[Ed Lee :Mardak]

Oh, I see from bug 1184933 comment 16 that the back button wasn't useful in the error view. Is it useful in this edit view (where there's also a Cancel button)?

Comment 3

[Sevaan Franks [:sevaan]]

(In reply to Ed Lee :Mardak from comment #2)
> Oh, I see from bug 1184933 comment 16 that the back button wasn't useful in
> the error view. Is it useful in this edit view (where there's also a Cancel
> button)?

We can leave it out; it's vestigial from some previous thinking.

Comment 4

[Ed Lee :Mardak]

Attachment: v1

We'll split out the live preview mentioned in comment 0 as it's turning out to be quite complicated with chat window dynamic resizing, etc.

One thing with the new design is that there's no simple way to select the default context and reverting to no context.

Comment 5

[Ed Lee :Mardak]

Attachment: v1 screenshot

Comment 6

[Ed Lee :Mardak]

Wasn't sure if the textarea should be resizable or not. Simple css change to remove: "resize: none;"

Comment 7

[Sevaan Franks [:sevaan]]

I would say not resizeable.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8661703 [details] [diff] [review]
v1

Seeing if Dan can review this for me...

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Sorry for not getting to this today, I should be able to get to it Thursday.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8661703 [details] [diff] [review]
v1

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

No matter how I apply this (and I've tried going back a bunch of commits and applying it there), I can't make it look like the v1 screenshot.  Whatever I do, I see the toolbar rendering on top of the edit context view, though it's not obvious to me why the changes you've made here would do that.

I'll work with you more on this tomorrow mid-morning.  In the mean-time, can you debug this and address the comment I've added thus far?

::: browser/components/loop/content/shared/css/conversation.css
@@ +1085,3 @@
>  }
>  
> +/* Buttons */

From what I can tell, all this button stuff appears to be copy-pasted from panel.css.  If that's true, can you instead simply move it from panel.css into common.css?

Comment 11

[Ed Lee :Mardak]

Gah sorry. I thought I landed this with bug 1199120 but just realized the tree was closed earlier and my push didn't go through.

Comment 12

[Ed Lee :Mardak]

Attachment: v2

Moved from panel/conversation to common.css.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8662797 [details] [diff] [review]
v2

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

Looks good; r=dmose

::: browser/components/loop/content/shared/css/conversation.css
@@ +970,5 @@
>  .showing-room-name > .text-chat-entries > .text-chat-scroller > .context-url-view-wrapper {
>    padding-top: 0;
>  }
>  
>  .room-context {

This _might_ want to be renamed to edit-room-context for clarity.

@@ +976,2 @@
>    border-top: 2px solid #444;
>    border-bottom: 2px solid #444;

Right now, the border looks odd and doesn't match the mockups, which appear to have none, so maybe setting this to 0 on both top and bottom is the right plan.

::: browser/components/loop/content/shared/js/views.jsx
@@ +797,5 @@
>        if (!this.props.showContextTitle) {
>          return null;
>        }
>  
> +      return <p>{mozL10n.get("context_inroom_label2")}</p>;

Need to make sure that the l10n file in standalone content/l10n/en-US is updated to match.

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ -893,5 @@
> -        expect(view.state.availableContext.previewImage).to.eql(favicon);
> -
> -        var node = view.getDOMNode();
> -        expect(node.querySelector(".checkbox-wrapper").classList.contains("hide")).to.eql(true);
> -      });

Check conversation.css to see if it has any checkbox-related stuff that should be removed too.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +348,1 @@
>  # title/URL of the website you are having a conversation about, displayed on a

Instead of "title/URL", I suspect "title and domain". context_inroom_header wants its own l10n comment explaining what it is and how it's different from this one.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Please spin out new bugs on the underline-on-hover style leaking into the text area, as well as the fact it allows you to enter a non-URL rather doing the typical input styling thing where it turns red and you have to fix it.

Comment 15

[Pulsebot]

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

Comment 16

[Mark Banner (:standard8)]

Ed: I've landed the patch to get the string change landed in time for the uplifts. I addressed the review comments about the string, you might need to revisit the ones about the css. Also, I suggest spinning .room-context -> .edit-room-context out to a separate bug, I'd like to see it done soon though as it is super confusing atm.

Comment 17

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/53c86ba9d1054adb8129348b17b97c60ea687e39
Bug 1184940 - Implement the refreshed design for the edit context view [r=dmose]

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/53c86ba9d105

Comment 20

[Bogdan Maris, Desktop QA]

Attachment: Separator gone in Nightly 44

Did some exploratory testing around this implementation using latest Developer Edition 43.0a2 across platforms (Windows 7 64-bit, Mac OS X 10.10.5, Ubuntu 14.04 32-bit) and did not found any new issues.

Also took a look at latest Nightly 44.0a1 and the separator between Edit context view and Chat seems to be gone.

Comment 21

[Ed Lee :Mardak]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #20)
> Created attachment 8668376 [details]
> Separator gone in Nightly 44
Yup. The new design didn't have a separator line.

Comment 22

[Bogdan Maris, Desktop QA]

(In reply to Ed Lee :Mardak from comment #21)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #20)
> > Created attachment 8668376 [details]
> > Separator gone in Nightly 44
> Yup. The new design didn't have a separator line.

Yes, you are right, I just received the mockups yesterday. Sorry for the noise.