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

Added invite option to room's context menu #5648

Merged
merged 6 commits into from
Mar 19, 2021

Conversation

jaiwanth-v
Copy link
Contributor

@jaiwanth-v jaiwanth-v commented Feb 15, 2021

Added invite option to room's context menu

ezgif com-gif-maker (2)

Signed-off-by: Jaiwanth [email protected]

@t3chguy t3chguy requested a review from a team February 15, 2021 13:51
@jaiwanth-v jaiwanth-v force-pushed the add-invite-to-context-menu branch from b6a63e2 to f420c85 Compare February 15, 2021 13:52
@jaiwanth-v jaiwanth-v force-pushed the add-invite-to-context-menu branch from 5b09da7 to 04d2bf1 Compare February 15, 2021 15:25
@jaiwanth-v
Copy link
Contributor Author

ezgif com-gif-maker (3)

@jaiwanth-v jaiwanth-v requested review from t3chguy and removed request for a team February 15, 2021 15:43
@SimonBrandner
Copy link
Contributor

@jaiwanth-v, could you please change the description so there isn't fixes <issue number>, so that this doesn't close the whole issue

@jaiwanth-v
Copy link
Contributor Author

Okay

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.

Code wise looks good other than the small nit where this.props.room && can be omitted.

Pending design review to determine if it is something we want

src/components/views/rooms/RoomTile.tsx Outdated Show resolved Hide resolved
@t3chguy t3chguy requested a review from a team February 16, 2021 13:02
@niquewoodhouse
Copy link
Contributor

Hi @jaiwanth-v I think this is a really good suggestion - thank you very much :) I'm one of the designers on the team, just taken a quick look and have a couple of suggested amends and a question.

Can I just confirm please if the option appears on private and public rooms? It's useful for both but I couldn't be sure from the gifs if its present for both. Thanks.

Some design proposed changes
I would like to propose some changes that might make the experience a bit easier for users:

  1. Copy changes (in the context menu)
  • Invite to this room Invite People (I've used a capital P just because the other things in the context menu use capital case for each word, for consistency)
  1. Copy changes (in the invite dialog)
  • Invite to this room Invite to room name

Reasons

  1. I realise 'invite to this room' is the text used in the member list panel but its quite wordy for a small context menu which could scale to have another 4 options one day - the shorter all the text is (especially when translated), the easier it is to scan. I personally think 'invite to this room' generally is too much anyway but its really highlighted in a small context menu that's trying to be small.

  2. Some people, depending on the room's sensitivity, will feel re-assured by seeing the room's name in the title of the invite dialog. There will be some users that would open, see dialog, close, re-open just to be sure it's the right room.

Thanks, let me know if you have any issues/questions/follow up for me.

@jaiwanth-v
Copy link
Contributor Author

Hi @niquewoodhouse, yes the invite option appears both on private and public rooms. The proposed changes look good. I'll make them soon.

@jaiwanth-v jaiwanth-v force-pushed the add-invite-to-context-menu branch from d57fd53 to 70e0b77 Compare March 19, 2021 16:03
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 otherwise (again)

src/components/views/rooms/RoomTile.tsx Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member

t3chguy commented Mar 19, 2021

Thanks for your contribution!

@t3chguy t3chguy merged commit 16ae01a into matrix-org:develop Mar 19, 2021
@jaiwanth-v jaiwanth-v deleted the add-invite-to-context-menu branch March 20, 2021 08:35
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.

4 participants