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

Initial support for editing messages #2952

Merged
merged 49 commits into from
May 15, 2019
Merged

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented May 9, 2019

Initial support for editing messages. Includes a new editor (that still needs some improvements) for editing. No formatting/markdown support yet (e.g. formatting is lost upon save).

Reviewer: The commit history is a bit convoluted, so would recommend to review all changes at once. Let me know if this is too big, and I can restructure the commits.

edit4

element-hq/element-web#9487
element-hq/element-web#9481
element-hq/element-web#9575

Accompanying js-sdk PR: matrix-org/matrix-js-sdk#913

A few words about how the new editor works:
On input events, the contents of the <div contenteditable> gets converted to a string. (textnodes are concatenated and block elements are turned into \n). Then we find out where the caret is in this string.
With these two pieces of information, the editor model is updated. It will first diff the new text value with the previous, to learn where text has been added and removed. The editor model is a list of parts that represent different formatting, restrictions, etc ... for inline elements. The diff is then applied to the parts, and after this is done, the model calculates where the caret should be now.

After this the model is reapplied to the DOM of the contenteditable element and the caret is repositioned. This has the advantage of being a catch-all for every kind of interaction that causes an input event, and also not relying in keyboard events. Hence, this should work better with IME input methods, etc..

react messes up the DOM sometimes because of, I assume, not
being aware of the changes to the real DOM by contenteditable.
componentWillReceiveProps doesn't run after mount,
and is deprecated as well.

Update state after both on componentDidMount and componentDidUpdate
also move part creation out of model, into partcreator, which
can then also contain dependencies for creating the auto completer.
e.g. pills, they get deleted when any character of them is removed
later on we also shouldn't allow the caret to be set inside of them
this way rendering is centralized and we can better rerender
from interaction in the autocompleter
(we didn't have access to caret before)
close autocomplete, and also replace with plain text part.

also remove leftover logging
@bwindels bwindels changed the title Message editing UI Initial support for editing messages May 14, 2019
@bwindels bwindels marked this pull request as ready for review May 14, 2019 15:05
@bwindels bwindels requested a review from a team May 14, 2019 15:05
@bwindels
Copy link
Contributor Author

Build seems to be failing because olm can't be fetched. Someone is looking at this ftr.

so we can have fallback content in the regular content for clients
that don't support edits. Note that we're not reading m.new_content
yet as it's going to be a bit of a headache to change this.

So for now just sending the edit in both the normal content and
the m.new_content subfield, so all events out there already
are well-formed
@turt2live turt2live self-requested a review May 14, 2019 17:07
Copy link
Member

@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.

generally lgtm - a bunch of small stuff to resolve

res/css/views/elements/_MessageEditor.scss Outdated Show resolved Hide resolved
res/css/views/elements/_MessageEditor.scss Outdated Show resolved Hide resolved
src/components/views/elements/MessageEditor.js Outdated Show resolved Hide resolved
src/components/views/elements/MessageEditor.js Outdated Show resolved Hide resolved
src/components/views/elements/MessageEditor.js Outdated Show resolved Hide resolved
src/components/views/elements/MessageEditor.js Outdated Show resolved Hide resolved
src/components/views/elements/MessageEditor.js Outdated Show resolved Hide resolved
}

onTab() {
//forceCompletion here?
Copy link
Member

Choose a reason for hiding this comment

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

unresolved TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. This is not fully featured yet, but I want to get this in front of people testing it sooner rather than later.

case "@": {
const displayName = completion.completion;
const userId = completion.completionId;
return new UserPillPart(userId, displayName);
Copy link
Member

Choose a reason for hiding this comment

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

what about @room?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, not supported yet.

src/settings/Settings.js Show resolved Hide resolved
res/img/edit.svg Outdated Show resolved Hide resolved
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