Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix mentions rendering in comment editor #35187

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Fix mentions rendering in comment editor #35187

merged 1 commit into from
Nov 21, 2022

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Nov 15, 2022 •
edited
Loading

NcRichContentEditable needs an index of users to properly display them[0].
This commit adds a caching logic and provides it to NcRichContentEditable.

[0] https://github.com/nextcloud/nextcloud-vue/blob/01dc781d18e2d2a81fed6ad578b90532709b7e1d/src/mixins/richEditor/index.js#L106-L119

Fix #35047

Screenshot from 2022-11-16 14-18-05

@artonge artonge enabled auto-merge November 15, 2022 17:31
@artonge artonge self-assigned this Nov 15, 2022
@artonge artonge added this to the Nextcloud 26 milestone Nov 15, 2022
@danxuliu
Copy link
Member

This is already addressed in #35053, but it was not reviewed yet :-)

@artonge
Copy link
Contributor Author

artonge commented Nov 15, 2022

Closing in favor of #35053

@artonge artonge closed this Nov 15, 2022
auto-merge was automatically disabled November 15, 2022 22:03

Pull request was closed

@artonge
Copy link
Contributor Author

artonge commented Nov 16, 2022

Reopening after discussion with @danxuliu :)

@artonge artonge reopened this Nov 16, 2022
@artonge artonge force-pushed the artonge/fix/mention_in_comment_editor branch from 6c303e4 to 80768c2 Compare November 16, 2022 09:24
@danxuliu danxuliu mentioned this pull request Nov 16, 2022
Closed
@artonge artonge enabled auto-merge November 16, 2022 13:18
miaulalala
miaulalala approved these changes Nov 18, 2022
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

LGTM

@miaulalala
Copy link
Contributor

/backport to stable25

@miaulalala
Copy link
Contributor

/backport to stable24

danxuliu
danxuliu approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

One thing, though. It seems that editing a comment that ends with a mention in Firefox does not work (or, at least, it does not work here :-) ):

  • Use Firefox
  • Send a new comment which is just a mention (like @user0)
  • Edit that comment
  • The cursor does not appear, and typing is not reflected in the comment. On the other hand, if the message was hello @user0 it is possible to place the cursor at the beginning with the mouse and type there, but not at the end.

But I guess this is not a problem caused by this pull request (well, it is because now there are rendered mentions, but you know what I mean ;-) ).

apps/comments/src/views/Comments.vue Show resolved Hide resolved
@danxuliu
Copy link
Member

/backport to stable23

@artonge
Fix mentions rendering in comment editor
0f66806 
NcRichContentEditable needs an index of users to properly display them. This commit adds a caching logic and provides it to NcRichContentEditable.

Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the artonge/fix/mention_in_comment_editor branch from 80768c2 to 0f66806 Compare November 21, 2022 09:49
@artonge artonge disabled auto-merge November 21, 2022 09:49
@artonge artonge mentioned this pull request Nov 21, 2022
Open
@artonge artonge merged commit e644205 into master Nov 21, 2022
@artonge artonge deleted the artonge/fix/mention_in_comment_editor branch November 21, 2022 11:24
@artonge
Copy link
Contributor Author

artonge commented Nov 21, 2022

/backport to stable25

@artonge
Copy link
Contributor Author

artonge commented Nov 21, 2022

/backport to stable24

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

1 similar comment
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

1 similar comment
@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

This was referenced Nov 22, 2022
Merged
Merged
@danxuliu danxuliu mentioned this pull request Nov 23, 2022
Merged
@edward-ly edward-ly mentioned this pull request Aug 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danxuliu danxuliu danxuliu approved these changes

@miaulalala miaulalala miaulalala approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@PVince81 PVince81 Awaiting requested review from PVince81

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees

@artonge artonge

Labels
3. to review Waiting for reviews enhancement feature: comments
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.

[Bug]: comment_editor text field shows UID instead of user mention_bubble
3 participants
@artonge @danxuliu @miaulalala