1168841 - Style the text chat display elements and add the time

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

This applies to both the link-generator and link-clicker display.

We've got the basic elements, but we need to style them as per the UX. See the user story for details.

Comment 1

[Mark Banner (:standard8)]

svg files on bug 1138445.

Comment 2

[Andrei Oprea [:andreio]]

Attachment: Style text chat elements and add timestamps

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8626185 [details] [diff] [review]
Style text chat elements and add timestamps

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

Here's an initial set of comments.


There's a few eslint errors raised:

content/shared/js/textChatView.jsx
   25:6   error  Prop types declarations should be sorted alphabetically  react/jsx-sort-prop-types
   26:6   error  Prop types declarations should be sorted alphabetically  react/jsx-sort-prop-types
   69:13  error  Missing parentheses around multilines JSX                react/wrap-multilines
  198:23  error  Missing parentheses around multilines JSX                react/wrap-multilines
  199:38  error  Props should be sorted alphabetically                    react/jsx-sort-props
  202:38  error  Props should be sorted alphabetically                    react/jsx-sort-props

::: browser/components/loop/content/shared/css/conversation.css
@@ +1520,5 @@
> +}
> +
> +.sent p,
> +.received p {
> +  background: #fff;

The selectors for this aren't very efficient and they could apply to other things not in the text-chat-entry tree... can you use .text-chat-entry > p ?

@@ +1524,5 @@
> +  background: #fff;
> +}
> +
> +.text-chat-entry.sent > p {
> +  border-bottom-right-radius: 0;

This needs a html[dir="rtl"] equivalent for left-radius and to reset the right radius.

There's a checkbox at the top of the ui-showcase for rtl mode where you can check/test these.

@@ +1528,5 @@
> +  border-bottom-right-radius: 0;
> +}
> +
> +.text-chat-entry.received > p {
> +  border-top-left-radius: 0;

Ditto wrt rtl.

@@ +1559,5 @@
> +  align-self: center;
> +}
> +
> +/* Sent text chat entries should be on the right */
> +.sent {

nit: again, these should apply to more specific items that just .sent or .received.

@@ +1563,5 @@
> +.sent {
> +  justify-content: flex-end;
> +}
> +
> +.received .text-chat-entry-timestamp {

Can we use the more efficient child selector please.

@@ +1578,5 @@
> +  background: #fff;
> +  content: "";
> +}
> +
> +.text-chat-entry.sent > p:after {

rtl modes needed for this and the .received equivalents.

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +27,5 @@
> +    },
> +
> +    getDefaultProps: function() {
> +      return {
> +        timestamp: (new Date()).toISOString()

If timestamp is required, I don't think we should be defaulting it here.

@@ +52,5 @@
> +
> +    /**
> +     * Pretty print timestamp. From time in milliseconds to HH:MM
> +     */
> +    _renderTimestamp: function() {

Can you put this above the render() function - we generally keep render() last as its easier to find.

@@ +55,5 @@
> +     */
> +    _renderTimestamp: function() {
> +      var date = new Date(this.props.timestamp);
> +      var minutes = date.getMinutes();
> +      var hours   = date.getHours();

nit: no need to align = here.

@@ +66,5 @@
> +        hours = "0" + hours;
> +      }
> +
> +      return <span className="text-chat-entry-timestamp">
> +        {hours}:{minutes}

The problem with using this is that it isn't going to adjust per a locale. I would suggest using something like:

new Date().toLocaleTimeString(navigator.mozL10n.language.code, {hour: "numeric", minute: "numeric", hour12: false})

::: browser/components/loop/jar.mn
@@ +40,5 @@
>    # one?
>    content/browser/loop/shared/img/spinner.png                   (content/shared/img/spinner.png)
>    content/browser/loop/shared/img/[e-mail]                (content/shared/img/[e-mail])
> +  content/browser/loop/shared/img/chatbubble-arrow-left.svg                (content/shared/img/chatbubble-arrow-left.svg)
> +  content/browser/loop/shared/img/chatbubble-arrow-right.svg                (content/shared/img/chatbubble-arrow-right.svg)

Can yuou align the ( with the ones above/below please

::: browser/components/loop/test/shared/textChatView_test.js
@@ +131,5 @@
> +        React.createElement(loop.shared.views.chat.TextChatEntry, props));
> +    }
> +
> +    beforeEach(function() {
> +    });

nit: empty function.

@@ +205,5 @@
> +      expect(node.querySelectorAll(".text-chat-entry-timestamp").length)
> +          .to.eql(1);
> +    });
> +
> +    it("should have a correctly formatted timestamp", function() {

I'm not sure we actually need to test this - this is more a function of the ui-showcase (especially when we change to localised values).

Comment 4

[Andrei Oprea [:andreio]]

Attachment: Style text chat elements and add timestamps

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8626302 [details] [diff] [review]
Style text chat elements and add timestamps

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

r+ with the comments addressed.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1518,5 @@
> +  align-self: auto;
> +}
> +
> +.sent > p,
> +.received > p {

Please add additional scoping e.g. to .text-chat-entry.sent (several places in the file).

::: browser/components/loop/content/shared/js/actions.js
@@ +186,5 @@
>       */
>      ReceivedTextChatMessage: Action.define("receivedTextChatMessage", {
>        contentType: String,
> +      message: String,
> +      receivedTimestamp: String

nit: please add in the fact that sentTimestamp is optional here.

::: browser/components/loop/test/shared/textChatView_test.js
@@ +268,5 @@
> +        sentTimestamp: "1970-01-01T00:02:00.000Z",
> +        receivedTimestamp: "1970-01-01T00:02:00.000Z"
> +      });
> +
> +      fakeClock.tick(1000 * 60);

I don't think you need this - given that you're specifying timestamps on all the entries.

Comment 6

[Andrei Oprea [:andreio]]

Attachment: Style text chat elements and add timestamps

Comment 7

[Pulsebot]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Bogdan Maris, Desktop Test Engineering]

I did some exploratory testing across platforms (Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit) using latest Aurora 41.0a2 and can confirm that the time layout changes and timestamp are visible and correctly displayed.