This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
Message editing: render avatars for pills in the editor #2997
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this way it won't interfere with editor selection/caret
so width of editor stay the same as content (unrelated to pills, really)
jryans
suggested changes
May 20, 2019
| @@ -20,7 +20,7 @@ limitations under the License. | |||
| // this is to try not make the text move but still have some | |||
| // padding around and in the editor. | |||
| // Actual values from fiddling around in inspector | |||
| margin: -7px -10px -5px -10px; | |||
| margin: -7px 24px -5px -10px; // right margin: -10px + 34px | |||
There was a problem hiding this comment.
I wonder if this could be done in a less fragile way...
There was a problem hiding this comment.
(Not a blocking comment as part of the review, I am just wondering...)
There was a problem hiding this comment.
I applied mx_EventTile_content to the editor, which has the 34px of margin. I had to override the overflow so the autocompletion doesn't get clipped, but I agree this is still better, because less magic in the numbers.
| @@ -37,8 +37,8 @@ function parseHtmlMessage(html) { | |||
| const resourceId = pillMatch[1]; // The room/user ID | |||
| const prefix = pillMatch[2]; // The first character of prefix | |||
| switch (prefix) { | |||
| case "@": return new UserPillPart(resourceId, n.textContent); | |||
| case "#": return new RoomPillPart(resourceId, n.textContent); | |||
| case "@": return new UserPillPart(resourceId, n.textContent, room.getMember(resourceId)); | |||
There was a problem hiding this comment.
Can room ever be null here?
There was a problem hiding this comment.
No, it's the current room. If you can write events (to be able to edit them later), the current room should always be an instance of Room AFAIK.
this way we don't have to include it in the magic number
the avatar style code is still different though, as it's implemented differently This also prevents updating the css variables when not needed, which caused the avatar to flicker when updating the editor.
this way, the background isn't clipped at the top
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note that this doesn't use
PilllikeMessageComposerInputbecause the editor contents isn't rendered by React. It moves most of the logic toAvatarthough, if it isn't there already, to avoid duplication to some extent.It's implemented as a pseudo-element on the span, with css variables set in JS passing the avatar url and letter down from the span. This way, it's sure to not interfere with the caret in the editor like an
<img>would.Part of element-hq/element-web#9487