Style the text chat display elements and add the time

VERIFIED FIXED in Firefox 41

Status

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

People

(Reporter: standard8, Assigned: andreio)

Tracking

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

Firefox Tracking Flags

(firefox41 verified)

Details

(Whiteboard: [chat])

User Story

Bug 1138445 has the Hello elements attached (currently attachment 8608752 [details] - in the "messages thread section). The bug also has example chat layouts.

Todo:

- Style the existing sent & received text elements in the manner described in the layout
- Add a timestamp to the text message
- Display the timestamp according to the style shown

Behavior details:

- Every minute a timestamp is inserted next to a speech bubble.
- The timestamp only appears once per minute and any subsequent messages during that minute do not have a timestamp.

Not part of this bug:

- The "day" marker

Attachments

(1 attachment, 2 obsolete attachments)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Rank: 20
Whiteboard: [chat]
User Story: (updated)
Rank: 20 → 12
Priority: P2 → P1
svg files on bug 1138445.
Assignee: nobody → andrei.br92
Attachment #8626185 - Flags: review?(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/spinner@2x.png                (content/shared/img/spinner@2x.png)
> +  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).
Attachment #8626185 - Attachment is obsolete: true
Attachment #8626185 - Flags: review?(standard8)
Attachment #8626302 - Flags: review?(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.
Attachment #8626302 - Flags: review?(standard8) → review+
Attachment #8626302 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/dd1fe44cfbec
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Iteration: --- → 41.3 - Jun 29
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.