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.

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

FIX: revert previously removed mentions transformation on the client #23084

Conversation

AndrewPrigorshnev
Copy link
Contributor

@AndrewPrigorshnev AndrewPrigorshnev commented Aug 14, 2023

This partially reverts 2ecc829.

The problem is that if we don't transform mentions right away, there is a noticeable lag before a mention gets fully rendered:

Screen.Recording.2023-08-14.at.19.00.22.mov

even when running locally:

Screen.Recording.2023-08-14.at.18.56.07.mov

While with this transformation, everything is super smooth:

Screen.Recording.2023-08-14.at.18.56.40.mov

I'm reverting that change only for mentions. Another part was about category hashtags, but unfortunately they lag both with and without this transformation. We need to address them separately.

cc @martin-brennan

@github-actions github-actions bot added the chat PRs which include a change to Chat plugin label Aug 14, 2023
@AndrewPrigorshnev AndrewPrigorshnev changed the title fix/revert removed mentions transformation when copking message on the client FIX: revert previously removed mentions transformation on the client Aug 14, 2023
@AndrewPrigorshnev AndrewPrigorshnev marked this pull request as ready for review August 14, 2023 15:05
@jjaffeux
Copy link
Contributor

Will let @martin-brennan look at this one given he changed this code few days ago

@martin-brennan
Copy link
Contributor

Ah my bad @AndrewPrigorshnev I didn't read properly and see simpleCategoryHashMentionTransform was doing mentions too 🤦 As for the hashtag render lag, I will have to have a think about it. We already do something like this in the composer preview, but I know that needs to do a /hashtags/lookup API call and we don't want to introduce this overhead here. At the very least I could just bring back the old code and only make it render the grey placeholder with the square next to it, but not actually make it into a URL etc. . I don't want something that is too much of an outlier from the rest of the hashtag code. Will tag you in the review when I do it 👍

@AndrewPrigorshnev
Copy link
Contributor Author

AndrewPrigorshnev commented Aug 15, 2023

@martin-brennan, great, thank you for looking into the hashtag thing! And could we merge this part for mentions first? May I have an approval on it?

Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

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

@AndrewPrigorshnev so sorry I didn't realise I didn't approve 🤦 Go ahead and merge!

@AndrewPrigorshnev
Copy link
Contributor Author

AndrewPrigorshnev commented Aug 22, 2023

@martin-brennan, no worries! Thank you for the review and the approval.

@AndrewPrigorshnev AndrewPrigorshnev force-pushed the fix/revert-removed-mentions-transformation-when-copking-message-on-the-client branch from 4e7c73d to b08ca91 Compare August 22, 2023 11:13
@AndrewPrigorshnev AndrewPrigorshnev merged commit 2d16446 into main Aug 22, 2023
13 checks passed
@AndrewPrigorshnev AndrewPrigorshnev deleted the fix/revert-removed-mentions-transformation-when-copking-message-on-the-client branch August 22, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat PRs which include a change to Chat plugin
3 participants