1183576 - Update chat styles

Status: RESOLVED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

Vicky has shown me a couple of small updates to the chat styles, including the context bubble:

https://usercontent.irccloud-cdn.com/file/FHO9Zcik/chat.png

https://usercontent.irccloud-cdn.com/file/suOMezfC/chatII.png

This bugs is all about getting these proposed changes incorporated.

Comment 1

[Marina Rodríguez [:mai]]

Attachment: bug-1183576.patch

Comment 2

[Marina Rodríguez [:mai]]

Comment on attachment 8633472 [details] [diff] [review]
bug-1183576.patch

Another patch for you, would you mind taking a look?
Best regards,
Marina

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8633472 [details] [diff] [review]
bug-1183576.patch

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

General comment: please don't forget to add a commit message.

The r- means that it's close enough that I believe one more iteration should be all that's necessary to complete this patch.
If only minor issues and/ or nits are left, an r+ is in order 'with comments addressed'.

But most awesome is, of course, a plain r+ ;-)

::: browser/components/loop/content/css/panel.css
@@ +242,5 @@
>    background-image: url("../shared/img/check.svg#check-blue");
>  }
>  
>  .new-room-view > .context > .checkbox-wrapper > label {
> +  color: #333333;

nit: please shorten this to `#333`

(I quote code between backticks - `<code>` - and sometimes denote it with the language its written in)

::: browser/components/loop/content/shared/css/conversation.css
@@ +1464,5 @@
>  
>  .text-chat-entry {
>    display: flex;
>    flex-direction: row;
> +  margin-right: 2px;

There are two (small) issues with this approach:

1) the margin looks wrong for RTL locales
2) the margin is not expressed in the more flexible 'em' unit

I propose to change the margin to `.2em` and add the following:

```css
html[dir="rtl"] .text-chat-entry {
  margin-right: auto;
  margin-left: .2em;
}
```

@@ +1488,3 @@
>    background: #fff;
>    word-wrap: break-word;
>    word-wrap: break-word;

oops! can you remove this duplicate statement while you're here?

@@ +1672,5 @@
>    border: 0;
> +  border-top: 1px solid #d8d8d8;
> +}
> +
> +.text-chat-box > form > input::-webkit-input-placeholder {

You're repeating quite a bit here; you might want to write this as:

```css
.text-chat-box > form > input::-webkit-input-placeholder,
.text-chat-box > form > input::-moz-placeholder,
.text-chat-box > form > input:-moz-placeholder,
.text-chat-box > form > input:-ms-input-placeholder,
.text-chat-box > form > input:input-placeholder {
  font-size: 1.1em;
  color: #999;
}
```

You can see that I moved to ems for the font-size and shortened the color hex value.

::: browser/components/loop/content/shared/img/chatbubble-arrow-right.svg
@@ +4,5 @@
>   <desc>Created with Sketch.</desc>
>   <g>
>    <title>Layer 1</title>
>    <g id="svg_1" fill="none">
> +   <path id="svg_2" fill="#5cccee" d="m19.505243,8.972466l-9.299002,0l0,-1l6.088001,0c-2.110002,-0.967 -4.742001,-2.818 -6.088001,-6.278l0.932,-0.363c2.202,5.664 8.377999,6.637 8.44,6.646c0.259001,0.039 0.444,0.27 0.426001,0.531c-0.019001,0.262 -0.237,0.464 -0.498999,0.464l-12.072001,-0.352"/>

Oy! This SVG file looks bad. Not your fault - of course - but can you file a bug to fix that?

Comment 4

[Marina Rodríguez [:mai]]

It’s important to use each declaration as a separated declaration. The block:

  .text-chat-box > form > input::-webkit-input-placeholder,
  .text-chat-box > form > input::-moz-placeholder,
  .text-chat-box > form > input:-moz-placeholder,
  .text-chat-box > form > input:-ms-input-placeholder,
  .text-chat-box > form > input:input-placeholder {

doesn't work because if the browser doesn't understand a rule, ignore/skip the block.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Marina Rodríguez [:mai] from comment #4)
> It’s important to use each declaration as a separated declaration. The block:
> 
>   .text-chat-box > form > input::-webkit-input-placeholder,
>   .text-chat-box > form > input::-moz-placeholder,
>   .text-chat-box > form > input:-moz-placeholder,
>   .text-chat-box > form > input:-ms-input-placeholder,
>   .text-chat-box > form > input:input-placeholder {
> 
> doesn't work because if the browser doesn't understand a rule, ignore/skip
> the block.

Can you show me proof of this assertion?

Comment 6

[Marina Rodríguez [:mai]]

Attachment: bug-1183576_r1.patch

New patch

Comment 7

[Marina Rodríguez [:mai]]

http://codepen.io/gitmai/pen/aOKaOK --> Shows the behaviour of CSS block with the placeholder rules

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8633990 [details] [diff] [review]
bug-1183576_r1.patch

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

Thanks, this looks good! r=me with comments addressed.

Please post a new patch with changes you made here before you set the 'checkin-needed' keyword.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1538,5 @@
>  }
>  
>  /* Text chat entry timestamp */
>  .text-chat-entry-timestamp {
> +  margin: 0 5px;

please make this `.5em`

@@ +1677,5 @@
> +  border-top: 1px solid #d8d8d8;
> +}
> +
> +.text-chat-box > form > input::-webkit-input-placeholder {
> +  font-size: 12px;

Please make this `1.1em` here and the ones below.

Comment 9

[Marina Rodríguez [:mai]]

Attachment: bug-1183576_r2.patch

Final patch

Comment 10

[Pulsebot]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_parsable_css.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=3773246&repo=fx-team

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

Comment 12

[Marina Rodríguez [:mai]]

Attachment: bug-1183576_r3.patch

Comment 13

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27f3b45f62b3

Comment 14

[Pulsebot]

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8634652 [details] [diff] [review]
bug-1183576_r3.patch

Approval Request Comment
[Feature/regressing bug #]: Hello text chat in conversation
[User impact if declined]: Visuals have received a ui-review and the necessary stylistic modification were made by this patch. It just makes the chat bubbles look that much better.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor, CSS-only change.
[String/UUID change made/needed]: n/a.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8634652 [details] [diff] [review]
bug-1183576_r3.patch

Patch seems safe, let's land the fix in Aurora.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e55f82397f32