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

[MM-12898] Allow in-line markdown images to open preview window #3639

Merged

Conversation

@abdusabri
Copy link
Contributor

abdusabri commented Sep 11, 2019

Summary

Updates the behavior of in-line markdown images to allow for opening the preview window. There are 3 cases as follows:

  1. If the image source is unsafe, a link is rendered
  2. If the image source is safe, the image will have a pointer cursor & a hover effect, and clicking on it will open the preview window
  3. If the image is a link itself, the image will be rendered and clicking on it will open the link in a new browser window (no preview or hover effect)

Here is a screen capture demonstrating the expected behavior.

inline-markdown-8

Ticket Link

Fixes mattermost/mattermost-server#9805

Copy link
Member

esethna left a comment

Thanks @abdusabri! Looking really good just one note,

  1. We should try to follow the UI that we use for attachments in terms of the hover state:

image
Attachment image ^

image
In-line image ^


FYI I filed a related bug ticket that I found while testing but it's not related to this PR so won't block merging this: https://mattermost.atlassian.net/browse/MM-18508

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Sep 12, 2019

Oh, thanks @esethna for catching that!

image

You might notice that the outer shadow has slightly less depth at the bottom compared to attached images, and that's is because the inline nature of markdown images has naturally less space/margins. When I tried the default (similar to attachments), it was a bit too much, specially with small icons like Github's favicon, in some other trials the ending was cut a little bit and didn't look natural.

Hopefully that's Ok and I didn't miss something 🙂

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Sep 12, 2019

@saturninoabril I will push a single commit with both of your comments implemented after #3647 is merged

Copy link
Member

esethna left a comment

Looks great, thanks @abdusabri! Great work on this, nice work finding and solving for the corner cases. Definitely have a browse at all the other tickets we have available if something catches your eye: https://github.com/mattermost/mattermost-server/issues?q=is%3Aopen+is%3Aissue+label%3A%22Help+Wanted%22

@esethna esethna removed the 1: PM Review label Sep 13, 2019
@saturninoabril

This comment has been minimized.

Copy link
Member

saturninoabril commented Sep 13, 2019

@abdusabri fyi, said PR is now merged.

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Sep 14, 2019

@esethna while updating the tests, I noticed that I'd removed the alt attribute of the images by mistake. Sorry, I've fixed this now.

I would like also to note that attached images don't have an alt attribute, I think this should be fixed under a separate ticket.

@abdusabri abdusabri requested a review from saturninoabril Sep 14, 2019
Copy link
Member

saturninoabril left a comment

In RHS, there's no hover effect.
In center view, the image seemed to be cut-off by a few pixels in standard mode (good in compact).
Screen Shot 2019-09-16 at 7 14 09 PM

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Sep 16, 2019

In RHS, there's no hover effect.
In center view, the image seemed to be cut-off by a few pixels in standard mode (good in compact).
Screen Shot 2019-09-16 at 7 14 09 PM

Thanks @saturninoabril , didn't know about this case, will fix it. (Getting to know the product 🙈)

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Sep 18, 2019

> FYI I filed a related bug ticket that I found while testing but it's not related to this PR so won't block merging this: https://mattermost.atlassian.net/browse/MM-18508

@esethna I'm afraid it is actually a regression issue. Made some progress investigating it, and will continue working on a fix.

@saturninoabril I've already fixed the styles for RHS case, and updated E2E tests. But i think it is best to wait until the above issue is fixed before finally pushing, agree?

Update: I've fixed the issue, and will wrap things up and push the updates later today. The regression issue is a variation of the above issue. A message like ![brokenlink](brokenlink) will render a broken image, and this is what I've fixed in my implementation. The above issue is till open.

@sudheerDev sudheerDev self-assigned this Sep 24, 2019
@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Sep 24, 2019

Thanks for the feedback @sudheerDev!

@sudheerDev sudheerDev merged commit 9e47782 into mattermost:master Sep 24, 2019
11 checks passed
11 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build-docker Your tests passed on CircleCI!
Details
ci/circleci: i18n-check Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prepare-docker-build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: upload-s3 Your tests passed on CircleCI!
Details
cla/mattermost abdusabri authorized
Details
mattermost/serverless-ops/github PR is up-to-date.
untagged-build Workflow: untagged-build
Details
@sudheerDev sudheerDev removed their assignment Sep 24, 2019
@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Sep 24, 2019

I would like also to note that attached images don't have an alt attribute, I think this should be fixed under a separate ticket.

@esethna, in case you missed ☝️

@amyblais amyblais added this to the v5.18.0 milestone Sep 24, 2019
jwilander added a commit that referenced this pull request Sep 24, 2019
* Allow in-line markdown images to open preview window

* Update markdown_image E2E tests

* Update inline markdown_image hover style

* Fix inline MD image RHS styles & broken link issues, update E2E tests

* Tweak css border rule, adjust px value in MD image E2E test
saturninoabril added a commit to saturninoabril/mattermost-webapp that referenced this pull request Sep 27, 2019
…ermost#3639)

* Allow in-line markdown images to open preview window

* Update markdown_image E2E tests

* Update inline markdown_image hover style

* Fix inline MD image RHS styles & broken link issues, update E2E tests

* Tweak css border rule, adjust px value in MD image E2E test
skheria added a commit to uber-uchat/mattermost-webapp that referenced this pull request Oct 3, 2019
…ermost#3639)

* Allow in-line markdown images to open preview window

* Update markdown_image E2E tests

* Update inline markdown_image hover style

* Fix inline MD image RHS styles & broken link issues, update E2E tests

* Tweak css border rule, adjust px value in MD image E2E test
skheria added a commit to uber-uchat/mattermost-webapp that referenced this pull request Oct 3, 2019
…ermost#3639)

* Allow in-line markdown images to open preview window

* Update markdown_image E2E tests

* Update inline markdown_image hover style

* Fix inline MD image RHS styles & broken link issues, update E2E tests

* Tweak css border rule, adjust px value in MD image E2E test
skheria added a commit to uber-uchat/mattermost-webapp that referenced this pull request Oct 3, 2019
…ermost#3639)

* Allow in-line markdown images to open preview window

* Update markdown_image E2E tests

* Update inline markdown_image hover style

* Fix inline MD image RHS styles & broken link issues, update E2E tests

* Tweak css border rule, adjust px value in MD image E2E test
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Nov 8, 2019
  * There was a change with inherited classes mattermost#3639
    on  markdown-inline-img causing this issue
  * Changing svg place holder to inherit width
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Nov 8, 2019
  * There was a change with inherited classes mattermost#3639
    on  markdown-inline-img causing this issue
  * Changing svg place holder to inherit width
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Nov 8, 2019
  * There was a change with inherited classes mattermost#3639
    on  markdown-inline-img causing this issue
  * Changing svg place holder to inherit width
deanwhillier added a commit that referenced this pull request Nov 11, 2019
* There was a change with inherited classes #3639
    on  markdown-inline-img causing this issue
  * Changing svg place holder to inherit width
mattermost-build added a commit to mattermost-build/mattermost-webapp that referenced this pull request Nov 11, 2019
  * There was a change with inherited classes mattermost#3639
    on  markdown-inline-img causing this issue
  * Changing svg place holder to inherit width
sudheerDev added a commit that referenced this pull request Nov 11, 2019
* There was a change with inherited classes #3639
    on  markdown-inline-img causing this issue
  * Changing svg place holder to inherit width
@lindy65 lindy65 added the Tests/Done label Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.