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

Conversation

@weeman1337
Copy link
Contributor

@weeman1337 weeman1337 commented Feb 21, 2021

closes element-hq/element-web#16182

Open points:

  • The IconizedContextMenu items support grouping. For now I have just put them into an order that looks useful to me. I am completely open for suggestions.
  • I have chosen the icons by best guess. Some are taken from the existing images. The others from Feather icons.

Looks quite good:

  • The user gets direct feedback which item he has selected
  • Icons

message_context

@weeman1337 weeman1337 marked this pull request as draft February 21, 2021 18:24
@weeman1337 weeman1337 changed the title WiP: Migrate message context menu to IconizedContextMenu Migrate message context menu to IconizedContextMenu Feb 21, 2021
@weeman1337 weeman1337 marked this pull request as ready for review February 24, 2021 21:16
@t3chguy t3chguy requested a review from a team February 25, 2021 10:44
@niquewoodhouse niquewoodhouse self-assigned this Apr 15, 2021
@niquewoodhouse
Copy link
Contributor

Design review

@weeman1337 Thank you so much for this contribution.

### Ordering

In the product today, the destructive options are at the bottom of other context menus. Remove should be at the end of the list.

Please can the order be as follows:

  1. Quote
  2. Forward
  3. Share
  4. Report
  5. Source
  6. Remove (if an option)

This order is optimized for continuing discussion within Element - the first options being ones that enable further communication.

Iconography

I have chosen the icons by best guess. Some are taken from the existing images. The others from Feather icons.

Just a few points. I went through the element icons to try to find appropriate ones. I personally prefer a few of the feather ones (e.g quote looks more familiar in your icons, share feels better), but consistency first and then changing the icon globally later.

Please use these element icons:

  • Quote has an icon already in element-icons/room/format-bar quote
  • Share has an icon in element-icons/room share
  • Remove has an icon in element-icons trashcan
  • View source (you might already be using it, but just saying incase) has an icon in element-icons/format-bar code
  • Report content, I think it should be warning-badge from element-icons. Don't think it needs to be in red, so please colour as other icons in the list. The icon currently used in the Netlify is used to mean "advanced settings", would be good to avoid using the same icon to mean different things.
  • There isn't a pre-existing icon for forward, but it should be visually a mirror of reply, the different arrows don't play well together.
    image
    I see the icons in the context menu are 16px, and I think the element icons are 18px? I made a forward icon here at 18px, to replace the forward being used. Let me know if I'm wrong about the sizes/alterations needed. Please could this icon be placed in the message-bar folder with the reply.svg?

fwd.svg.zip

Misc

image

There appears to be no bottom border on the penultimate item. It looks like maybe it's because two menuitems are within the same IconizedContextMenu_optionList causing this?

I totally appreciate this is not necessarily part of your PR but it might help improve the menu a tiny bit. The menu text is a bit long-winded/inconsistent. For example, quote is just a quote but forward is forward message and share is share message. Can it just be share and forward? It makes the list slightly less heavy. I also don't think report content needs to be anything more than report?

Please feel free to mention me if there's any questions/when I can take another look. Will be sure to stay on top of this issue to get it through review asap.

Thanks again for this change!

@ShadowJonathan
Copy link

@niquewoodhouse wouldn't it be a good idea that the "remove" option has its icon and text in red?

@weeman1337
Copy link
Contributor Author

weeman1337 commented May 25, 2021

Thanks for the reply @niquewoodhouse

Here is how it currently looks like:

image

I have updated the icons and labels according to your suggestions.

Regarding the order and grouping the context menus have two levels: Groups and items. Unfortunately the original screenshot in the PR description doesn't show all the items because some are bound to a condition. See my screenshot in this comment for all the items. I can reorder them as you like.

The icons sizes of the context menu icons are currently 16px. Like for example in the "main menu":

image

(The warning-badge icon looks huge but it actually is 16x16px. Most of the other items reach the 16px limit either horizontally or vertically what makes them look smaller.)

@niquewoodhouse
Copy link
Contributor

@weeman1337 Thanks so much for the response with all the items that could be there, really helps to see what needs doing in a comprehensive view like that.

I put them in a big list (please ignore any styling differences between this design and the existing context menu)

Kapture 2021-06-04 at 14 45 19

I guessed that Pin becomes Unpin, and that Unhide becomes its opposite Hide, just included them as different ones for reference.

Icons

(The warning-badge icon looks huge but it actually is 16x16px. Most of the other items reach the 16px limit either horizontally or vertically what makes them look smaller.)

Ah, that sucks. I didn't make the icons and just expected them to be able to sit nicely together. The icons are actually a bit painful generally at the moment now I look at them a bit more.

Attached is a new warning badge to replace the old one, it should fit a bit nicer now. It's fine to replace the existing warning badge with that icon. There's a link icon in element-icons for the Source URL bu that's 18x18, it might scale down ok. There's a retry icon in element-icons for Resend (0) reactions - although it's 18x19 (randomly) so might come out looking odd. There's a hide icon in element-icons we could use for Hide preview, there's an appearance icon in element-icons/settings we could use for Show preview. And I think that covers them all.

We're definitely keen to redraw icons to be a) more understandable generally b) have a clear consistent relationship with eachother (eg even just being the same size would be great) but it's just going to push this change further back. So I'm guessing some icons might still look a bit strange, but they should now look not too strange and we should fix them all separately.

Order of options & groups and items

I don't understand the logic of the groups and items. I'd like to amend their order to be like this longer-term:

image

But I'd suggest retaining whatever ordering it has for now. I imagine being used to, say, collapse thread being underneath forward, and now I can't find it...it's annoying. There's already enough change happening.

I'd just suggest removing the borders completely from the context menu, or just having 1 border above the remove option. I find all the borders make it slightly hard to compare the options because it makes you skip some/distract you from comparing icons/reading labels.

Thanks for this contribution!

warning-badge.svg.zip

@weeman1337 weeman1337 force-pushed the msg-context-menu branch 2 times, most recently from 7a1344c to 901047d Compare June 22, 2021 18:23
@weeman1337
Copy link
Contributor Author

@niquewoodhouse I have just had some time to implement your suggestions.

Here is what it looks now (if all items are being displayed):
MessageContext

The line above remove is more or less required because the colour is bound to the respective group (groups are separated by lines).

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Mostly LGTM code-wise, just some styling

if (redactButton) {
optionLists.push((
<IconizedContextMenuOptionList key={'group2'} red>
{redactButton}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{redactButton}
{ redactButton }

@ShadowJonathan
Copy link

The line above remove is more or less required because the colour is bound to the respective group (groups are separated by lines).

Even if it's possible to remove this, id say to keep it in, it makes the separation of all other "progressive" actions vs this "destructive" action even stronger.

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Regarding the styling. There already is a rule set up in https://github.com/matrix-org/eslint-plugin-matrix-org to which Element is slowly migrating. The js-sdk already uses it but the react-sdk doesn't, yet.

Copy link
Contributor

@niquewoodhouse niquewoodhouse left a comment

Choose a reason for hiding this comment

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

lgtm, thank you so much for your patience in going through a couple of rounds of feedback on this.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

LGTM other than one small code style nit

@SimonBrandner
Copy link
Contributor

CI seems unhappy

@weeman1337
Copy link
Contributor Author

CI seems unhappy

I have rebased the branch - it is happy now 😄

@weeman1337 weeman1337 requested a review from t3chguy June 26, 2021 08:01
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Thanks!

@t3chguy
Copy link
Member

t3chguy commented Jun 28, 2021

Unfortunately, some of your commits are missing the sign-off, any chance you could add it to the PR description?

Thanks

Signed-off-by: Michael Weimann <[email protected]>
@weeman1337
Copy link
Contributor Author

weeman1337 commented Jun 28, 2021

Unfortunately, some of your commits are missing the sign-off, any chance you could add it to the PR description?

I have added it to the commits.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-implementation of the message context menu

5 participants