Status

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mikedeboer, Assigned: mai)

Tracking

unspecified
mozilla42
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

(Whiteboard: [chat])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

4 years ago
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.
Flags: qe-verify-
Flags: firefox-backlog+
Assignee

Updated

4 years ago
Assignee: nobody → marina.rodrigueziglesias
Assignee

Comment 1

4 years ago
Posted patch bug-1183576.patch (obsolete) — Splinter Review
Assignee

Comment 2

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

Another patch for you, would you mind taking a look?
Best regards,
Marina
Attachment #8633472 - Flags: review?(mdeboer)
Reporter

Comment 3

4 years ago
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?
Attachment #8633472 - Flags: review?(mdeboer) → review-
Assignee

Comment 4

4 years ago
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.
Reporter

Comment 5

4 years ago
(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?
Assignee

Comment 6

4 years ago
Posted patch bug-1183576_r1.patch (obsolete) — Splinter Review
New patch
Attachment #8633472 - Attachment is obsolete: true
Attachment #8633990 - Flags: review?(mdeboer)
Assignee

Comment 7

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

Comment 8

4 years ago
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.
Attachment #8633990 - Flags: review?(mdeboer) → review+
Assignee

Comment 9

4 years ago
Posted patch bug-1183576_r2.patch (obsolete) — Splinter Review
Final patch
Attachment #8633990 - Attachment is obsolete: true
Attachment #8634088 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 12

4 years ago
Attachment #8634652 - Flags: review?(mdeboer)
Reporter

Updated

4 years ago
Attachment #8634088 - Attachment is obsolete: true
Reporter

Updated

4 years ago
Attachment #8634652 - Flags: review?(mdeboer) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Iteration: --- → 42.2 - Jul 27
https://hg.mozilla.org/mozilla-central/rev/eae92a786a16
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Reporter

Comment 16

4 years ago
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.
Attachment #8634652 - Flags: approval-mozilla-aurora?
Comment on attachment 8634652 [details] [diff] [review]
bug-1183576_r3.patch

Patch seems safe, let's land the fix in Aurora.
Attachment #8634652 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.