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

Improved forwarding UI #5999

Merged
merged 42 commits into from
Jun 8, 2021
Merged

Conversation

robintown
Copy link
Member

@robintown robintown commented May 9, 2021

This replaces the old forwarding UI with a dialog based on designs from element-hq/element-web#14690.

Forwarding text message in sent and failed states
Forwarding image in sending state

Closes element-hq/element-web#5641.
Closes element-hq/element-web#7230.
Closes element-hq/element-web#10165.
Closes element-hq/element-web#14690.
Closes element-hq/element-web#17109.

Replaces the old forwarding UI with a dialog based on designs from
element-hq/element-web#14690.

Signed-off-by: Robin Townsend <[email protected]>
Signed-off-by: Robin Townsend <[email protected]>
This has been replaced by ForwardDialog.

Signed-off-by: Robin Townsend <[email protected]>
@robintown
Copy link
Member Author

robintown commented May 9, 2021

Note that since support for verifiable forwarded messages is blocked on spec, this PR does not attempt to make any changes to how forwarded messages appear.

@SimonBrandner
Copy link
Contributor

SimonBrandner commented May 9, 2021

A few design nitpicks:

  • It would be nice if there was more space between the buttons and the scrollbar

Screenshot_20210509_074940

  • Maybe it would look nice if there was a border around the EventTilePreview?
  • Also the avatar/room name could be clickable and send you to that room

Otherwise, looks awesome! 🎉

Signed-off-by: Robin Townsend <[email protected]>
Previously ForwardDialog was not giving its EventTile message preview
the information it needed to render a ReplyThread. This was a bit tricky
to fix since we were pulling a fake event out of thin air, so this
ensures it doesn't regress.

Signed-off-by: Robin Townsend <[email protected]>
It makes things shorter and more readable!

Signed-off-by: Robin Townsend <[email protected]>
…and add a tooltip to explain why they can't accept forwarded messages.
It was chosen to disable the buttons rather than hide the entries from
the list, since hiding them without explanation could cause confusion.

Signed-off-by: Robin Townsend <[email protected]>
Signed-off-by: Robin Townsend <[email protected]>
@t3chguy t3chguy requested a review from a team May 10, 2021 09:37
@DoM1niC
Copy link

DoM1niC commented May 10, 2021

bigger String cut the Btn
image

Nice PR btw 👍🏻 I like it

…to give more space between it and the buttons.

Signed-off-by: Robin Townsend <[email protected]>
…so that you can jump to a room easily once you've forwarded a message
there.

Signed-off-by: Robin Townsend <[email protected]>
@robintown
Copy link
Member Author

robintown commented May 10, 2021

It would be nice if there was more space between the buttons and the scrollbar

  • Done

bigger String cut the Btn

  • I tried increasing my font size and switching languages but couldn't reproduce this, anyways it should be fixed by the above change

Maybe it would look nice if there was a border around the EventTilePreview?

  • This wasn't in the original designs but it is something to consider. Unfortunately overflow: scroll seems to break border-radius (though I could probably work around that), here's what it would look like anyways:

message forwarding preview with a border

Also the avatar/room name could be clickable and send you to that room

  • Done, I assume the use case here is jumping to a room right after you send a message to there, which seems pretty important

@t3chguy t3chguy self-requested a review May 19, 2021 10:41
@DoM1niC
Copy link

DoM1niC commented May 19, 2021

Could you sort the list by recent chat activity ? I forward often media content and have to search every user who wrote recently :( THX anyway

@robintown
Copy link
Member Author

@DoM1niC That's already the intended behavior, is it behaving differently for you?

@robintown
Copy link
Member Author

Oh, I assume you're referring to the fact that direct messages are at the end of the list in a separate section. Now that I look at it again, the original design doesn't seem like it kept them separate, so I'll update this to match.

@niquewoodhouse
Copy link
Contributor

Thanks @t3chguy, based on that info, I don't think making the header change of adding an info tooltip is worth it.

@t3chguy
Copy link
Member

t3chguy commented May 24, 2021

@niquewoodhouse presumably that means you agree that forwarding should strip any reply-to metadata then to prevent that guaranteed permissions error?

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.

Thank you so much @robintown, this is such a substantially better and more understandable experience. Really looking forward to testing once shipped, getting more feedback, and iterating if/when needed.

@niquewoodhouse
Copy link
Contributor

@niquewoodhouse presumably that means you agree that forwarding should strip any reply-to metadata then to prevent that guaranteed permissions error?

I'm not sure I do agree to be honest, but it might be because I lack some specific knowledge at this time. I'd like to separate things out anyway, this PR is about improving the forwarding UI, I think it does a great job of that.

The actual thing being forwarded/the logic defining it is maybe separate - I don't want to hold this up to work that out. Presumably, this PR hasn't touched that pre-existing logic? That might come up organically from this being shipped and further issues coming up.

@t3chguy
Copy link
Member

t3chguy commented May 24, 2021

Sure it is separate but given we're polishing Forwarding, and forwarding a message (which is a reply) to another room will ALWAYS yield that error because replies can only reply to within that same room and a forward is a copy-paste of the event JSON.
I doubt there's any contention on the subject, but up to you

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.

Looks awesome otherwise

res/css/views/dialogs/_ForwardDialog.scss Outdated Show resolved Hide resolved
src/components/views/context_menus/MessageContextMenu.js Outdated Show resolved Hide resolved
src/components/views/dialogs/ForwardDialog.tsx Outdated Show resolved Hide resolved
src/components/views/dialogs/ForwardDialog.tsx Outdated Show resolved Hide resolved
src/components/views/dialogs/ForwardDialog.tsx Outdated Show resolved Hide resolved
src/components/views/dialogs/ForwardDialog.tsx Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member

t3chguy commented Jun 1, 2021

It feels a bit strange that you can forward to the same room

image

@robintown
Copy link
Member Author

robintown commented Jun 1, 2021

@t3chguy It feels a bit strange that you can forward to the same room

I actually find that quite useful, for example if you're trying to dig up an old message for someone

Signed-off-by: Robin Townsend <[email protected]>
Signed-off-by: Robin Townsend <[email protected]>
...to dynamically watch for layout changes.

Signed-off-by: Robin Townsend <[email protected]>
This also allows us to filter by room aliases.

Signed-off-by: Robin Townsend <[email protected]>
Signed-off-by: Robin Townsend <[email protected]>
@DoM1niC
Copy link

DoM1niC commented Jun 5, 2021

last changes on develop have a lot of conflicts :(

Update: recent commits fixed this THX @robintown

As TypeScript points out, you can only set an id on HTML elements, not
arbitrary components.

Signed-off-by: Robin Townsend <[email protected]>
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.

This is great, just one tiny lint nit :)

src/components/views/dialogs/ForwardDialog.tsx Outdated Show resolved Hide resolved
It was a bullet point, since EventTiles now get created as li by default
:P

Signed-off-by: Robin Townsend <[email protected]>
Signed-off-by: Robin Townsend <[email protected]>
since trying to interact with them is pointless.

Signed-off-by: Robin Townsend <[email protected]>
@t3chguy t3chguy merged commit 432373a into matrix-org:develop Jun 8, 2021
@HarHarLinks
Copy link
Contributor

@robintown I actually find that quite useful, for example if you're trying to dig up an old message for someone

It seems more intuitive to quote or even better reply to in that case? I don't really disagree that Element could offer this functionality, as long as it doesn't convolute the visible rooms/ordering I guess, I'm not sure what the status quo is.

Great work, I have been looking "forward" to this :)

@MurzNN
Copy link
Contributor

MurzNN commented Jun 10, 2021

@HarHarLinks, for implement right forwarding we need to accept the MSC2730: Verifiable forwarded events, as I understand.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants