Closed Bug 1215593 Opened 4 years ago Closed 4 years ago

Relayout the room title and context tile in text chat as part of the web sharing user journey

Categories

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

defect

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Iteration:
44.3 - Nov 2
Tracking Status
firefox45 --- fixed

People

(Reporter: standard8, Assigned: crafuse)

References

Details

(Whiteboard: [web sharing])

User Story

Acceptance Criteria:

- Room name and initial context tile on standalone look like the attached mock-up.
- The initial context tile is not shown on desktop any more.
- Desktop doesn't increase the conversation window height on open if the room has context.

Technical:

Right Column in Desktop mode width 250px.

Attachments

(11 files, 11 obsolete files)

52.25 KB, image/png
Details
16.14 KB, image/png
sevaan
: feedback-
Details
16.14 KB, image/png
Details
6.48 KB, image/png
Details
11.93 KB, image/png
sevaan
: feedback-
Details
25.41 KB, image/png
sevaan
: ui-review+
Details
36.58 KB, image/png
sevaan
: ui-review+
Details
30.68 KB, image/png
sevaan
: ui-review+
Details
46.13 KB, patch
standard8
: review+
Details | Diff | Splinter Review
22.85 KB, image/png
Details
24.89 KB, image/png
Details
Attached image Mock Up of layout
As part of the user journey rework, we need to change how the special chat tiles for room name and context tile.

See the user story for more detail.
Rank: 13
Blocks: 1215612
Assignee: nobody → chris.rafuse
Status: NEW → ASSIGNED
Attached image conversation context
Comment on attachment 8675840 [details]
conversation context

Does this context stay? Any other changes on this view?
Attachment #8675840 - Flags: feedback?(sfranks)
Shown after allowing camera and microphone.
Attachment #8675848 - Flags: feedback?(sfranks)
User Story: (updated)
Comment on attachment 8675848 [details]
post Allow Panel Current before updates

Obviously something is not right here :-/

Could you post a screenshot of what a notification on Linux normally looks like?
Attachment #8675848 - Flags: feedback?(sfranks) → feedback-
Comment on attachment 8675840 [details]
conversation context

Feedback given over Vidyo with Chris.
Attachment #8675840 - Flags: feedback?(sfranks) → feedback-
Comment on attachment 8677655 [details] [diff] [review]
Relayout the room title and context tile in text chat

Removed showRoomName as it is always shown if context is shown.
Added showInitialContext property to show context for link clicker's chat view.
adjusted context tile and chat entries to match the mock-up spacing and style.
removed context_inroom_label2 and context_inroom_header string properties as they are not needed.
Attachment #8677655 - Flags: review?(standard8)
Adjusted chat entry spacing to match 10px gutters of the context tile
Attachment #8677659 - Flags: ui-review?(sfranks)
Attachment #8677655 - Attachment is obsolete: true
Attachment #8677655 - Flags: review?(standard8)
Comment on attachment 8677659 [details]
chat views for link generator(no context) and clicker (context)

newer one available
Attachment #8677659 - Attachment is obsolete: true
Attachment #8677659 - Flags: ui-review?(sfranks)
Attachment #8677664 - Flags: review?(standard8)
Updated margin around room title
Attachment #8677672 - Flags: ui-review?(sfranks)
Comment on attachment 8677672 [details]
chat views for link generator(no context) and clicker (context)

Great, that's laid out better.

Is this the tile seems off to me though...an we reduce the top and bottom fadding by a couple px? Can that be done in this bug?
Attachment #8677672 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8677664 [details] [diff] [review]
Relayout the room title and context tile in text chat

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

I've not looked at the css in too much detail yet until we've got ui-review.

We do need to also fix the textChatStore#_appendTextChatMessage to not signal LoopChatMessageAppended on a special context entry. This will stop the whitespace appearing at the bottom of the conversation window.

::: browser/components/loop/content/shared/css/common.css
@@ +552,4 @@
>    /* Use the flex row mode to position the elements next to each other. */
>    display: flex;
>    flex-flow: row nowrap;
> +  /*line-height: 1.1em;*/

nit: please remove obsolete lines.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1127,3 @@
>    /* 18px for indent of .text-chat-arrow, 1px for border of .text-chat-entry > p,
> +   0.8rem for padding of .text-chat-entry > p */
> +  padding: 2rem 1rem;

This feels like too much padding to me, how about 1rem all around? (subject to UX) - its what we have now.

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +174,5 @@
>            <div className="text-chat-scroller">
>              {
>                this.props.messageList.map(function(entry, i) {
>                  if (entry.type === CHAT_MESSAGE_TYPES.SPECIAL) {
> +                  if (this.props.showInitialContext) {

The two if statements above can be combined into one.

Also, you'll need to add a test for this.

@@ +382,5 @@
>  
>      render: function() {
> +      var messageList = this.state.messageList;
> +      if (!this.props.showInitialContext) {
> +        messageList = messageList.filter(function(item) {

I don't think this filter is needed any longer, since you're doing the work in TextChatEntriesView#render
Attachment #8677664 - Flags: review?(standard8) → review-
(In reply to Sevaan Franks [:sevaan] from comment #14)
> Comment on attachment 8677672 [details]
> chat views for link generator(no context) and clicker (context)
> 
> Great, that's laid out better.
> 
> Is this the tile seems off to me though...an we reduce the top and bottom
> fadding by a couple px? Can that be done in this bug?

I will reduce the top and bottom padding and attach a new screen shot for ui-review.
Attached image Cleaned up spacing in chat view. (obsolete) —
Attachment #8679034 - Flags: ui-review?(sfranks)
Attachment #8679034 - Flags: ui-review?(sfranks) → ui-review+
Attached image chat View Popout menu screen capture (obsolete) —
Attachment #8677672 - Attachment is obsolete: true
Attachment #8679037 - Flags: ui-review?(sfranks)
Attachment #8677664 - Attachment is obsolete: true
Comment on attachment 8679037 [details]
chat View Popout menu screen capture

+R, but looking at this and the last screenshot, I think we should add some padding to the top of the chat in the conversation window, so there is always a little gap between the video area and the top of the chat bubble when scrolled to the top.

Is this the bug for that?
Attachment #8679037 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8679098 [details] [diff] [review]
Relayout the room title and context tile in text chat

Added test for show initial context set to false.
Fixed up CSS for padding and spacing issues.
Left combining of conditions as each condition returns differently.
Removed filtering of special, room titles and context types from message list.
Attachment #8679098 - Flags: review?(standard8)
Attachment #8679098 - Attachment is obsolete: true
Attachment #8679098 - Flags: review?(standard8)
Attachment #8679034 - Attachment is obsolete: true
Attachment #8679037 - Attachment is obsolete: true
Attachment #8679230 - Flags: ui-review?(sfranks)
Attachment #8679231 - Flags: ui-review?(sfranks)
Attached image text Chat no context
Added the space at the top of the text chat entries.
Attachment #8679234 - Flags: ui-review?(sfranks)
Comment on attachment 8679248 [details] [diff] [review]
Relayout the room title and context tile in text chat

Added test for show initial context set to false.
Fixed up CSS for padding and spacing issues.
Left combining of conditions as each condition returns differently.
Removed filtering of special, room titles and context types from message list.
Re-spaced context url wrapper to be margin instead of padding.
room-name-shown class removed when no context is shown.
Attachment #8679248 - Flags: review?(standard8)
Attachment #8679230 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8679231 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8679234 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8679248 [details] [diff] [review]
Relayout the room title and context tile in text chat

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

Looking nicer, though you've not fixed the whitespace issue I mentioned in comment 15 that appears when the conversation window is opened - so at the moment when no-one else is in the room, there's the extra whitespace appearing that isn't any use.

::: browser/components/loop/content/shared/css/common.css
@@ +590,5 @@
>  .clicks-allowed.context-wrapper:hover {
>    border: 2px solid #5cccee;
>    /* Due to the increased border width, reduce the padding accordingly so that
>       the text doesn't move. */
> +  /*padding: calc(.8em - 1px);*/

nit: Comments need removing.

::: browser/components/loop/content/shared/css/conversation.css
@@ +689,5 @@
>     fix its height. */
>  .media-wrapper > .text-chat-view {
>    flex: 0 0 auto;
>    /* Text chat is a fixed 200px width for normal displays. */
> +  width: 275px;

If you do this, then its going to mess up the local video display (and remote video when screen sharing) on popped-out windows, or on the standalone - you can see the broken displays quite easily in the showcase.

If you want to fix this here, then we need to maintain the 4:3 aspect ratio of the local video and the remote video when screen sharing - which is quite awkward, as the height works out to 206.25, so maybe UX will let us change the width a bit for more exact calculation ;-)

We could also spin the width change out to another bug - we need one to change the layout of the standalone when screen sharing so that the remote & local video are above the text chat and overlaid - so I'd be happy for us to address the width of the text chat area in that bug.

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +174,5 @@
>            <div className="text-chat-scroller">
>              {
>                this.props.messageList.map(function(entry, i) {
>                  if (entry.type === CHAT_MESSAGE_TYPES.SPECIAL) {
> +                  if (this.props.showInitialContext) {

Can you swap this around, so that we do the `if (!this.props.showInitialContext) { return null; }` first. We can then drop the else for the switch statement below.

That way, we get the simple case out the way first, and simplify the indentation of the rest.
Attachment #8679248 - Flags: review?(standard8) → review-
Comment on attachment 8679744 [details] [diff] [review]
Relayout the room title and context tile in text chat

Moved initial context condition.
Removed commented code.
Will keep width of 275px and will create new bug for resizing column width and text chat height for video screens.
Will attach bug to this as depends on.
Attachment #8679744 - Flags: review?(standard8)
Attachment #8679248 - Attachment is obsolete: true
Attachment #8679744 - Attachment is obsolete: true
Attachment #8679744 - Flags: review?(standard8)
Attachment #8679765 - Attachment is obsolete: true
Attachment #8680137 - Flags: review?(standard8)
Attachment #8680137 - Flags: review?(dmose)
Pairing with :standard8, we:

Corrected condition logic for filtering messages - reduced the number of messages in the message list(less data being created) and filtered out context and title entries on events(see next).

Removed test for LoopChatDisabledMessageAppended event as it does not trigger on initialization, now the message list is at zero length and does not trigger the event.  There is opportunity to remove dead code here.

Modified CSS for context tile margin.
Comment on attachment 8680137 [details] [diff] [review]
Relayout the room title and context tile in text chat

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

As mentioned on irc, I'd prefer to land this without the adjustment for 275px, and do that in the follow-up. The reason is, that otherwise we'd leave ourselves in a non-shippable state, which we generally prefer not to do.

The rest of this is ready to land though.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ -217,5 @@
>  
>  # LOCALIZATION NOTE (context_inroom_header): this string is displayed in the
>  # conversation window when the user edits context. It is a header to the edit
>  # section.
> -context_inroom_header=Let's Talk About…

Don't remove this one yet - we still need it for the edit context display.
Attachment #8680137 - Flags: review?(standard8)
Attachment #8680137 - Flags: review?(dmose)
Attachment #8680137 - Attachment is obsolete: true
Comment on attachment 8680704 [details] [diff] [review]
Relayout the room title and context tile in text chat

Un-removed string from properties files.
Removed resizing of column from 275px to the original 200px.
Tested and screen shots provided.
Attachment #8680704 - Flags: review?(standard8)
Attachment #8680704 - Flags: review?(dmose)
Had to remove the width change to another bug.
Comment on attachment 8680704 [details] [diff] [review]
Relayout the room title and context tile in text chat

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

Looks good, we still should be removing the l10n comments for the strings we did want to remove, but I can handle that on check-in
Attachment #8680704 - Flags: review?(standard8)
Attachment #8680704 - Flags: review?(dmose)
Attachment #8680704 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e68feee3233c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Iteration: --- → 44.3 - Nov 2
You need to log in before you can comment on or make changes to this bug.