Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Reintroduce "Create narrow mode for composer" #6753

Merged

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Sep 7, 2021

Reverts #6752
See #6682
Fixes element-hq/element-web#18533

Context: the original PR was lacking design review before it got landed, so starting a messy revert chain to bring it back. This PR is meant to be authored by @gsouquet


This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://613a016e4f18ac17d76da503--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@turt2live turt2live requested a review from a team as a code owner September 7, 2021 04:12
@turt2live turt2live requested a review from janogarcia September 7, 2021 04:12
@turt2live turt2live added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Sep 7, 2021
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on @janogarcia 's approve before merging this one out

@janogarcia
Copy link
Contributor

janogarcia commented Sep 7, 2021

@gsouquet Glad to see a first iteration happening! Thanks for working on it, Germain.

Some missing details and stuff that would need to be improved:

  • We should keep the file upload action always visible, even when the other actions are collapsed. Please refer to the mockup.
  • We should also preserve the encryption shield indicator (there's a chance we might decide to remove it before the first release, but let's keep it for now).
  • The placeholder text should read Reply to thread… instead of Send a message… or Send an encrypted message…
  • The tooltip message should read More options instead of Composer menu
  • OK, this one is new (I'll update the mockup accordingly). The labels for the actions in the popover menu should read as follows: Add emoji…, Send a sticker…, Send voice message…

Some bugs I ran into while testing it:

  • The collapsed actions are not triggering/opening their corresponding dialog when acting on them. (e.g. clicking on Show emojis or Show stickers won't have any effect).
  • Both composers (room and thread panel) are collapsed as soon as the the breakpoint for the thread composer kicks in. Each composer should only react to its own state.
  • This won't be relevant once we have updated the placeholder text, but I'm reporting it anyway as it may relate to other stuff. In an encrypted room I'm getting the expected placeholder text for the room composer (Send an encrypted message…) but not for the thread composer (Send a message...), which should match that of the room composer.

Copy link
Member Author

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@janogarcia janogarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the stuff in my previous comment. 👍

I would suggest keeping the placeholder text as just Reply to thread… instead of Reply to encrypted thread…. I guess it would feel less intimidating to a wider audience, plus we're already informing on the encryption status in the shield icon. That said, we'll get to discuss those copy details later on.

The label for the Emoji picker should read Add emoji. I'm not following the same copy for both actions, the Emoji and Sticker pickers, as those have slightly different outcomes. The Emoji picker just adds one or multiple emojis to the composer (but there's no implicit send action involved), while the Sticker picker will automatically send a sticker as soon as you pick one. Once again, the final copy is subject to be reviewed.

One last detail I forgot to mention in my last comment. Let's keep the hover style applied to the More options button while it's active and its popover is shown.

@janogarcia
Copy link
Contributor

@gsouquet Ahrg, forgot to mention this as well... It would nice to get rid of the right margin for .mx_MessageComposer_sendMessage when in the thread panel.

@germain-gg
Copy link
Contributor

I would suggest keeping the placeholder text as just Reply to thread… instead of Reply to encrypted thread….

This pattern is already used for the normal message placeholder. If an e2eStatus is given to the composer it will say "Send an encrypted message…" versus "Send a message" otherwise.
For that reason I'll probably keep it as is to not diverge too much. I mostly agree with you that it can feel intimidating but I am under the impression that copy exists for a strong reason

The label for the Emoji picker should read Add emoji.

Fixed, sorry that was an oversight on my side

Let's keep the hover style applied to the More options button while it's active and its popover is shown.

It would nice to get rid of the right margin for .mx_MessageComposer_sendMessage when in the thread panel.

Copy link
Contributor

@janogarcia janogarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @gsouquet ! 👍

Copy link
Member Author

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awkward approval goes here (please, github, let me approve of myself)

@germain-gg germain-gg merged commit d475b7f into develop Sep 9, 2021
@germain-gg germain-gg deleted the revert-6752-revert-6682-gsouquet/compact-composer-18533 branch September 9, 2021 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update composer to fit in thread view
3 participants