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

Add edit mode for messages (experimental) #588

Closed
wants to merge 1 commit into from
Closed

Conversation

pik
Copy link

@pik pik commented Dec 9, 2016

Intended to be the companion PR to element-hq/element-web#2725

Adds an MessageEditInput which is a basic wrapper around Editor to be triggered when edit action is received from dispatcher (triggered on context-menu click).

Missing:matrix-js-sdk methods that send an edit message to be functional.

Escape - cancels edit mode.
Enter - should send edit message.

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

// localize subscribe/dispatch?
if (payload.action === 'edit' && payload.mxEvent === this.props.mxEvent) {
this.state.editMode = !this.state.editMode
this.forceUpdate()
Copy link
Author

Choose a reason for hiding this comment

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

First time using React sorry about this =(

I assume there is somewhere I should register editMode so that changes to it trigger re-render.. (will-fix)

Copy link
Member

Choose a reason for hiding this comment

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

yup, indeed, 99% of the time calling forceUpdate() is a mistake.
in react, you never ever directly mutate a component's state; instead you go via this.setState({editMode: !this.state.editMode});, which does the necessary magic for the component to know it has to re-render itself.

@ara4n
Copy link
Member

ara4n commented Dec 22, 2016

this is looking very promising. the big concern I have is the duplication of MessageEditorInput relative to MessageComposerInput (and MessageComposerInputOld). It's bad enough that we have the pre-RTE and RTE composer duplication already, and adding another fork of the same codebase scares me to death, especially as the RTE composer still doesn't work. Instead, i'd be tempted to extend MessageComposerInput so that it can be used for in-place editing as well as normal entry (or alternatively consider a UX where 'editing' a message just lets you use the MessageComposer to edit the contents of said message)

@pik pik changed the title Add edit mode for messages (in progress) Add edit mode for messages (experimental) Dec 26, 2016
 * Adds pending state for pending edits
 * final state for complete edit
 * Adds MessageEditorInput Component (a highly simplified composer)
@pik pik force-pushed the edit branch 2 times, most recently from 8f381d3 to 61edf82 Compare December 26, 2016 12:48
@pik
Copy link
Author

pik commented Dec 26, 2016

Apologies I haven't had time to update this until very recently. Looking at the current MessageEditorInput and MessageComposerInput -- almost none of the code is duplicated - this is particularly because while both are wrappers around the draft-js Editor the MessageComposerInput supports a range of functions which are (I think) not needed for MessageEditorInput (particularly auto-complete on names, file-attachments etc.); events associated with key presses are also handled differently.

@pik
Copy link
Author

pik commented Dec 26, 2016

Also wondering if there has been some discussion on how message edits should be distinguished on the UI. atm the timestamp box is too small to include an (edited at <time>) message:
screenshot from 2016-12-26 10-04-44

@gonight
Copy link

gonight commented Jan 8, 2017

High-visibility, especially with theoretically-unlimited backlog as a feature (thus possibly many users searching/referencing), seems like a good approach. Slack's approach of following the edited message with a greyed-out '(edited)', which shows the edit date on mouseover seems like a good general approach. Down the line, as the feature matures, the '(edited)' could be made clickable, possibly to show previous versions of the message.

@vijfhoek
Copy link
Contributor

vijfhoek commented Jan 8, 2017

Maybe it'll be an idea to stop allowing edits after a certain amount of time has passed, cutting down on abuse.

@ghost
Copy link

ghost commented Aug 19, 2017

@SijmenSchoon but that wouldn't be good, because this would lock down the users' freedom. making it available every time would be better. how can the user abuse the editing function?

@anoadragon453
Copy link
Member

anoadragon453 commented Sep 2, 2017

Also wondering if there has been some discussion on how message edits should be distinguished on the UI. atm the timestamp box is too small to include an (edited at ) message:

Perhaps something like what reddit does for editing messages, by putting a * after the time stamp. More information will be available hovering over it on desktop, or tapping on mobile.

Normal:
2017-09-01-180148

Hover:
2017-09-01-180128

@anoadragon453
Copy link
Member

anoadragon453 commented Sep 2, 2017

I also believe privileged users (perhaps above a configurable level) being able to edit any messages would allow a lot of fun features for bots. Having an "edit history" showing previous edits and which user made new ones is also important to preserve a timeline of events.

@AlexCzar
Copy link

AlexCzar commented May 28, 2018

Is this still being worked on? Not being able to edit messages seems like a pretty big negative, coming from Slack.

@t3chguy
Copy link
Member

t3chguy commented May 28, 2018

@AlexCzar there's no support for it in matrix yet, as that is an immutable DAG

@ara4n
Copy link
Member

ara4n commented May 29, 2019

we now have edits via #2952 and similar. thanks @pik for being 3 years ahead of the curve - i'm sorry we couldn't merge this at the time :(

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.

8 participants