Closed Bug 1168848 Opened 5 years ago Closed 5 years ago

Style the text entry field for text chat

Categories

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

defect
Points:
2

Tracking

(firefox41 verified)

VERIFIED FIXED
mozilla41
Iteration:
41.3 - Jun 29
Tracking Status
firefox41 --- verified

People

(Reporter: standard8, Assigned: dmose)

References

Details

(Whiteboard: [chat])

User Story

Style as per the layouts on bug 1138445. See the "Hello elements" under the "Inputs" -> "IM field" section.

To implement:

- Idle/Inactive state
- Active state

Not included in this bug:

- The "link" and "emojis" buttons
- With link
- Emojis displayed

Attachments

(1 file, 7 obsolete files)

The text-entry field needs styling for text chat on both standalone & desktop displays.

See the user story for more information.
Flags: qe-verify+
Flags: firefox-backlog+
Rank: 20
Whiteboard: [chat]
Assignee: nobody → dmose
Iteration: --- → 41.2 - Jun 8
Rank: 20 → 12
Priority: P2 → P1
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
Depends on: 1168833
Based on top of patch in bug 1168848; also fixes FramedExample to handle
dashed examples correctly and adds dashes in various places so that one
can see where the TextChatView borders are.
Note that this does turn on the text chat view for all of the various full views in the showcase, but, given where we are in development, that seems reasonable to me.
Attachment #8621159 - Flags: review?(standard8)
This version updated to populate the message list and remove a regression.  It turns out we're going to need to fix bug 1173909 for this to be very useful, so I'm going to do that before requesting review.
Attachment #8617619 - Attachment is obsolete: true
Attachment #8621159 - Attachment is obsolete: true
Attachment #8621159 - Flags: review?(standard8)
Depends on: 1173909
This needs more post-rebase cleanup; should be straightforward.
Attachment #8623852 - Flags: review?(standard8)
Attachment #8621289 - Attachment is obsolete: true
Attachment #8623383 - Attachment is obsolete: true
Comment on attachment 8623852 [details] [diff] [review]
Make TextChat style correctly when focussed

I see how to make the scrolling simulate the real app code flow better; new patch forthcoming.
Attachment #8623852 - Attachment is obsolete: true
Attachment #8623852 - Flags: review?(standard8)
Comment on attachment 8623852 [details] [diff] [review]
Make TextChat style correctly when focussed

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

::: browser/components/loop/content/shared/css/conversation.css
@@ +1308,5 @@
>    border-top: 1px solid #999;
>  }
>  
> +/* turn the visible border blue as a visual indicator of focus */
> +.fx-embedded .text-chat-box > form > input:focus {

I'm pretty sure this shouldn't be desktop only. Please check with UX if unsure.
This patch turns on text-chat in the ui-showcase, in addition to fixing
the style user-story here.  It also a few other minor tweaks
As part of landing, I'll file a bug about fixing the "invitiation" view that
this leaves in an odd state...
Attachment #8624441 - Flags: review?(standard8)
Attachment #8624441 - Flags: review?(andrei.br92)
Comment on attachment 8624441 [details] [diff] [review]
Make TextChat style correctly when focussed

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

When you file the bug on the invitation display, please can you extend it to find a solution to display different states of the text chat store - especially on desktop, we should have a "text chat enabled but no entries visible" option (ideally we'd have variants for standalone as well, e.g. text chat not enabled).

::: browser/components/loop/content/shared/css/conversation.css
@@ +1499,5 @@
>  
> +/* turn the visible border blue as a visual indicator of focus */
> +.text-chat-box > form > input:focus {
> +  border: 0;
> +  border-top: 1px solid #66c9f2;

I think this needs ux-review/input.

On desktop I can barely see the difference especially when the entries field is collapsed.

Whilst this is as-designed, I'm not sure its good enough.

On Standalone, there's a 1px border all the way around that disappears (except for the top border) when you click in the element. If the placeholder is in there, then that shifts position as well.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +252,5 @@
>      sdkDriver: mockSDK
>    });
>  
>    textChatStore.setStoreState({
> +    textChatEnabled: true,

Please can you move the updateRoomInfo to before this section, so that the showcase displays more like what's expected (it'd be extremely unlikely that we'd get updateRoomInfo in after text messages - and there's a bug on file somewhere that will move us to ensure that we do).
Attachment #8624441 - Flags: review?(standard8) → review-
Blocks: 1176339
(In reply to Mark Banner (:standard8) from comment #9)

> When you file the bug on the invitation display, please can you extend it to
> find a solution to display different states of the text chat store -
> especially on desktop, we should have a "text chat enabled but no entries
> visible" option (ideally we'd have variants for standalone as well, e.g.
> text chat not enabled).

See bug 1176339.
 
> ::: browser/components/loop/content/shared/css/conversation.css
> @@ +1499,5 @@
> >  
> > +/* turn the visible border blue as a visual indicator of focus */
>
> I think this needs ux-review/input.
>
> On desktop I can barely see the difference especially when the entries field
> is collapsed.
> 
> Whilst this is as-designed, I'm not sure its good enough.

Had a Hello call with Sevaan about this, he pointed out that this is intended to be a subtler cue than an input box that requires input, since it's unclear when/if you'll type in this one again.  He suggested leaving it as designed for now, and he'll talk to Vicky about other options.
 
> On Standalone, there's a 1px border all the way around that disappears
> (except for the top border) when you click in the element. If the
> placeholder is in there, then that shifts position as well.

This is actually a separate bug: looking closely at the design, there shouldn't be 1px border all the way around in the unfocussed state ever.  I've fixed this in the new iteration.
 
> ::: browser/components/loop/ui/ui-showcase.jsx
> @@ +252,5 @@
> >      sdkDriver: mockSDK
> >    });
> >  
> >    textChatStore.setStoreState({
> > +    textChatEnabled: true,
> 
> Please can you move the updateRoomInfo to before this section, so that the
> showcase displays more like what's expected (it'd be extremely unlikely that
> we'd get updateRoomInfo in after text messages - and there's a bug on file
> somewhere that will move us to ensure that we do).

Fixed. This forced me to move to using actions rather than mucking about in the store directly, which is less fragile anyway.
Attachment #8624441 - Attachment is obsolete: true
Attachment #8624441 - Flags: review?(andrei.br92)
Attachment #8624871 - Flags: review?(standard8)
Attachment #8624871 - Attachment is obsolete: true
Attachment #8624871 - Flags: review?(standard8)
Attachment #8624874 - Flags: review?(standard8)
Comment on attachment 8624874 [details] [diff] [review]
Make TextChat style correctly when focussed

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

Ok, looks good. r=Standard8 with the comment addressed.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +296,3 @@
>    // Update the text chat store with the room info.
>    textChatStore.updateRoomInfo(new sharedActions.UpdateRoomInfo({
> +      roomName: "A Very Long Conversation Name",

This section doesn't need an extra indentation.
Attachment #8624874 - Flags: review?(standard8) → review+
Dan: I pushed this for you with my comment addressed, so that I could push the fix to bug 1176278.
Awesome; thanks!
https://hg.mozilla.org/mozilla-central/rev/1fab1b2a759b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: bogdan.maris
Tested using Firefox Developer Edition 41.0a2 and https://loop-dev.stage.mozaws.net/v0 across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) and I can confirm the implementation is according to user story, found no new issue so I'll mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.