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

[MM-37074] Encapsulate inserted GIFs in inline image markdown #8362

Merged
merged 2 commits into from
Jul 14, 2021
Merged

[MM-37074] Encapsulate inserted GIFs in inline image markdown #8362

merged 2 commits into from
Jul 14, 2021

Conversation

Roy-Orbison
Copy link
Contributor

@Roy-Orbison Roy-Orbison commented Jul 7, 2021

Summary

Changes Markdown inserted by GIF picker from

https://thumbs.gfycat.com/LikableValidAlpineroadguidetigerbeetle-size_restricted.gif

to

![Beverly Hills Cop - OK gesture](https://thumbs.gfycat.com/LikableValidAlpineroadguidetigerbeetle-size_restricted.gif)

Ticket Link

As per Other notes of [MM-12290]
https://mattermost.atlassian.net/browse/MM-37074

Screenshots

link only
inlined image

Release Note

Improved default rendering of images inserted via the GIF picker.

@mattermod
Copy link
Contributor

Hello @Roy-Orbison,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermod mattermod requested a review from devinbinnie July 7, 2021 05:31
@mattermod mattermod added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jul 7, 2021
@srkgupta srkgupta self-requested a review July 7, 2021 11:08
@srkgupta
Copy link
Contributor

srkgupta commented Jul 7, 2021

Adding myself to check the CodeQL alert created on this PR

Copy link
Contributor Author

@Roy-Orbison Roy-Orbison left a comment

Choose a reason for hiding this comment

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

Is this check somehow aware of Markdown syntax, thus expecting the .replace() to also escape possible \ in title properties? Or is it balking at the escaped [ in the regex? Or incorrectly parsing the escaped \ in the replacement string?

@devinbinnie devinbinnie requested review from hmhealey and removed request for devinbinnie July 7, 2021 13:51
@srkgupta
Copy link
Contributor

srkgupta commented Jul 8, 2021

Is this check somehow aware of Markdown syntax, thus expecting the .replace() to also escape possible \ in title properties? Or is it balking at the escaped [ in the regex? Or incorrectly parsing the escaped \ in the replacement string?

@Roy-Orbison the alert did not consider the Markdown syntax and hence it's a false positive. The changes LGTM and I have dismissed the alert.

@srkgupta srkgupta removed their request for review July 8, 2021 13:24
@hmhealey hmhealey added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jul 8, 2021
@hmhealey hmhealey requested a review from hahmadia July 8, 2021 21:12
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @Roy-Orbison! Good catch on the followup to that ticket since it doesn't look like one was created to actually track this separately

@@ -59,7 +59,7 @@ export default class GifPicker extends React.PureComponent {
}

handleItemClick = (gif) => {
this.props.onGifClick(gif.max5mbGif);
this.props.onGifClick('![' + gif.title.replace(/]|\[/g, '\\$&') + '](' + gif.max5mbGif + ')');
Copy link
Member

Choose a reason for hiding this comment

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

Also good call escaping the square brackets in the gif title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't escape \ characters a well, but that's because the flavour of markdown in MM simply looks for a \ preceding a square bracket. So [\\[x\]](#y) renders like this on GH [\x], but the first [\ of that rendering is included in the link text on MM.

Markdown is a little ill-defined.

Copy link
Member

Choose a reason for hiding this comment

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

We do plan on switching MM's Markdown to CommonMark which should match GitHub more closely, so we'll likely have to start doing that eventually, but I don't think it's a concern for now. The only way I see this becoming a concern is if Gfycat starts sending us bad data anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All user data is bad data ;)

@Roy-Orbison
Copy link
Contributor Author

I tested on that build instance, and the gif markdown worked in all three contexts that receive the updated function parameter (create post, edit post, create comment).

@hahmadia hahmadia removed the 2: Dev Review Requires review by a core commiter label Jul 12, 2021
@hmhealey hmhealey requested a review from jgilliam17 July 12, 2021 16:02
@jgilliam17 jgilliam17 changed the title Encapsulate inserted GIFs in inline image markdown [MM-12290] Encapsulate inserted GIFs in inline image markdown Jul 14, 2021
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

Thanks @Roy-Orbison
Tested, looks good to merge.

  • Verified GIF inline markdown

@jgilliam17 jgilliam17 changed the title [MM-12290] Encapsulate inserted GIFs in inline image markdown [MM-37074] Encapsulate inserted GIFs in inline image markdown Jul 14, 2021
@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jul 14, 2021
@mm-cloud-bot
Copy link

Test server destroyed

@jgilliam17 jgilliam17 merged commit afe71a9 into mattermost:master Jul 14, 2021
@amyblais amyblais added the Changelog/Done Required changelog entry has been written label Jul 14, 2021
@amyblais amyblais added the Docs/Not Needed Does not require documentation label Jul 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants