fix: Improve GIF handling and display consistency in chat messages #984
Conversation
|
|
KrishnaShuk
force-pushed
the
fix/improve-gif-url-handling
branch
from
c6a362c to
29751df
Compare
|
@Spiral-Memory I have made all the required changes and tested it. You can see in my latest commit. |
|
Sorry removing it asap! |
|
N make sure you format your code with prettier extension otherwise format check workflow will fail |
|
Removed all console logs and fixed the formatting of the files. |
|
Hey @Spiral-Memory! I have made the required changes. You can merge this PR. |
| if (url && url.includes('/file-upload/')) { | ||
| const fullUrl = url.startsWith('http') | ||
| ? url | ||
| : `${host}/file-upload/${url.split('/file-upload/')[1]}`; |
There was a problem hiding this comment.
Can you explain the reasoning behind checking whether http is present, then extracting the full URL and everything else
Can't we do it without these manual interventions
There was a problem hiding this comment.
The idea here was to handle two possible cases: absolute URLs (like https://example.com/file-upload/123) that are good to go as-is, and relative ones (like /file-upload/123) that need the host tacked on to work. I check for http to figure out which case I’m dealing with, so I don’t accidentally double up the host on an absolute URL or leave a relative one incomplete. The splitting part just grabs the file ID after /file-upload/ to rebuild the URL properly.
we could just assume everything’s relative and put host on front, though that’d break if an absolute URL slips in.
There was a problem hiding this comment.
Why there will be a difference ? I mean once the image is uploaded, it will have any one of these URLs right ? Have you observed anything that in what cases, there could be a chance of absolute / relative URLs
There was a problem hiding this comment.
No i haven't checked the cases. I have just guessed it. And you are right that once the image is uploaded it will have any one of these URLs. Should i make a commit in which url is put out directly without any condition?
There was a problem hiding this comment.
Yeah, check which kind of url is present after the upload and based on it, just use it
There was a problem hiding this comment.
The url is of relative type. Sould i totally remove the conditions logic or use this instead -> const fullUrl = ${host}/file-upload/${url.split('/file-upload/')[1]};
There was a problem hiding this comment.
Sure, pls go ahead with the new change and once you're done, request a re-review
Thanks a lot for the contribution 😌😊
There was a problem hiding this comment.
Thanks a lot @Spiral-Memory! These conversations surely help me in leveling up.
This PR addresses issues with GIF rendering and display consistency in chat messages.
1.Enhanced GIF detection
2.Implemented consistent sizing
3.Fixed animation handling
4.Improved URL handling
Acceptance Criteria fulfillment
Fixes #982
Before
After
PR Test Details
Note: The PR will be ready for live testing at https://rocketchat.github.io/EmbeddedChat/pulls/pr-<pr_number> after approval. Contributors are requested to replace
<pr_number>with the actual PR number.