[MM-15232] [MM-18158] Add a container to small images, Fix UX issues when markdown images are used in channel header #3877

abdusabri · 2019-10-06T00:54:35Z

Summary

Attached images ~~and in-line markdown images~~ with a width or height below 48px will be placed inside a container div to allow for better click/tap targeting.

Expected behavior:

The container will have a 1px border with rounded corners (4px)
The container will have a hover effect and a pointer cursor
The container will have the same background color as the center channel theme color
The enclosed image will have its own hover effect and rounded corners removed
Clicking on the container div will open the preview modal
If both width and height of the image are bigger than 48px, no container will be added
- If an in-line markdown image is a link itself, no container will be added (even if the width and/or height are below 48px, as it doesn't have a hover effect and doesn't open the preview modal, @esethna, please let me know if this should change)

Also fixes an issue where images with a big height didn't respect max height constraints, and adds hover effect for in-line markdown images if they are also a link

When markdown images are used in a channel header, they are:

Scaled down to fit the available height in the header description area
Scaled down in the system messages that states the header has been changed
Scaled down in the expanded header state (when the header is clicked)

Screenshots

Ticket Link

Fixes mattermost/mattermost#12187 and https://mattermost.atlassian.net/browse/MM-18158

andrewbrown00

Thanks @abdusabri, this is looking great 🎉

Few items to resolve

Inline images are being stretched when posted, this is a regression. Clicking on the image shows the image properly (not stretched).

Expected/current behavior
Inline images always show the hover effect, even if they are not a link.

@esethna what are your thoughts here, I'm 4/5 we should show the hover effect if the inline image is a also a link.

If an in-line markdown image is a link itself, no container will be added (even if the width and/or height are below 48px, as it doesn't have a hover effect and doesn't open the preview modal, @esethna, please let me know if this should change)

abdusabri · 2019-10-08T08:11:46Z

@andrewbrown00, Thanks for the feedback and for reporting the stretching issue of in-line markdown images! I've fixed it.

FYI, it is also happening on the daily version of the web app, but not on the community, I think this might be from my previous PR on MM-12898.

The cases for in-line markdown images are:

If the image source is unsafe, a link is rendered -> Not relevant for this PR
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 -> Covered
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) -> Will add hover effect and a container if width or height less than 48px as you suggested and Eric gave 👍 (but will still open the link, not the preview modal)

Inline images always show the hover effect, even if they are not a link.

As far as I understand, this is case 2. form above, and that is the expected behavior. Did I miss something?

Thanks! 🙂

esethna · 2019-10-08T16:18:13Z

@abdusabri apologies, I think I may have misspoke, I don't think inline markdown images should get a container at all. Given they are typically used in-line, it could be distracting to show a container around them if they are small. For example, here is how an in-line markdown image is used in the developers channel header, which wouldn't be possible if we added a container around the image

abdusabri · 2019-10-08T16:32:17Z

@esethna, no worries! Maybe i read between the lines a bit too much, sorry 😊. And i'm also not really convinced it should have a container 🙈 it didn't look nice at all when I tried it. Glad you confirmed.

So, i will only add hover effect for inline images that are a link themselves.

@andrewbrown00, i would be waiting for your confirmation if i missed something regarding your 2nd comment. Update: i will remove the container as well as Eric just explained to me

andrewbrown00 · 2019-10-08T17:40:28Z

@abdusabri thanks and Eric was correct and I missed that use case. Again, really nice job with this and I look forward to your future contributions 😄

abdusabri · 2019-10-08T23:08:05Z

@esethna, @andrewbrown00

Thanks for the image link example of the channel header use case, that gave me a new problem to solve 😄

Some special handling was needed to get the hover effect to work properly for the channel header.

Besides a couple of technical issues that I've resolved, the main consideration is the limited space (height) available in the header (22px max).

I set smaller max-height constraints for the image and created some space to allow for the hover effect. Here is an example of how it looks like.

I've separated this into its own commit in case input from Design/UX is needed so it won't block this PR in that case.

esethna

@abdusabri this is great! A few comments on the change to in-line images in the channel header (feel free to break this out into another PR if you feel necessary (your call), so we aren't creating scope creep since you are actually solving this ticket in here as well: https://mattermost.atlassian.net/browse/MM-18158):

We should respect the reduced size in the system message states that states the header has been changed
Inline images should always have rounded corners by default (otherwise there is a strange state where the rounded corners only appear on hover)
We should also respect the reduced size in the expanded header state (ie click in the header to see the expanded view):

abdusabri · 2019-10-09T22:51:28Z

Thanks for the feedback @esethna.

I'm totally fine fixing this in this PR. Turned out it is not a quick fix though 🙃, but I've a draft implementation already in place 🙂.

Here is a screenshot

I will need some more time to polish the implementation and add a bunch of tests 😄. When done, I will also update the PR description to state that it is fixing MM-18158 as well.

lieut-data · 2019-10-11T00:48:41Z

/check-cla

andrewbrown00

@abdusabri thanks for the updates, a few small changes listed below:

Inline image in the header doesn't look like it has rounded corners by default.

Note See ES comment

Inline images should always have rounded corners by default (otherwise there is a strange state where the rounded corners only appear on hover)

@esethna I'm 5/5 we should reduce the corner radius to 2px for small images, ie the inline header image. Here's what it looks like with:
2.1. 4px corners (current)

2.2. 2px proposed

abdusabri · 2019-10-14T14:35:45Z

@andrewbrown00, on my local environment rounded corners are working as expected - by default -, and the test server was not updated to the latest commit, so can't test there.

Here is what it looks like for me

Where are you getting this behavior? How can i reproduce?

andrewbrown00 · 2019-10-14T14:45:55Z

My apologies @abdusabri I saw this

and thought new changes we available. So I emptied my cache and did a hard reload on the existing test server.

I'll wait for an updated test server link 😉

Note I still like we should reduce the corner radius to 2px for smaller images, but I'll let Eric chime in.

abdusabri · 2019-10-14T14:51:51Z

No worries @andrewbrown00!

Once Eric confirms, i will reduce corners to 2px. In that case, it should also be applied to the scaled down images in the system messages. correct?

sudheerDev · 2019-11-20T17:07:44Z

@sudheerDev

For scroll pop issue of MD images, please check the below screenshots based on community daily.

@abdusabri Can You check this once again on daily? Or send me a sample of the post for me to cross check. I cannot replicate this anymore.

We had few issues and we fixed them with #4164 and #4206.

As far as width: 100% this will cause issues with inline images with text as they now take up 100% of the space causing then to start with multiline and then wrap to a single line.

abdusabri · 2019-11-21T09:12:20Z

@sudheerDev
For scroll pop issue of MD images, please check the below screenshots based on community daily.

@abdusabri Can You check this once again on daily? Or send me a sample of the post for me to cross check. I cannot replicate this anymore.

We had few issues and we fixed them with #4164 and #4206.

As far as width: 100% this will cause issues with inline images with text as they now take up 100% of the space causing then to start with multiline and then wrap to a single line.

@sudheerDev

Thanks for the feedback.

I can confirm that I can no longer repro the issue on master. I was able to repro 8 days ago because #4206 was not merged at that time.

I will sync with master and test again on my branch with width: auto - not 100% - to confirm everything is working as expected. But I'm not sure I will have time to do so in the next couple of days.

sudheerDev

LGTM!. Thanks @abdusabri

sudheerDev · 2019-11-21T09:44:43Z

@abdusabri The change of width you have here is same as master. Once you merge i believe you wouldn't need it here any longer. Rest all looks good to me so, went ahead and approved it.

devinbinnie

Looks good overall, just some small changes around CSS requested. Thanks @abdusabri!

devinbinnie · 2019-11-21T16:46:15Z

components/markdown_image.jsx

- alt={alt}
- src={brokenImageIcon}
- />
+ <div style={{display: 'inline-block'}}>


Can we convert this to a class instead of using an inline style?

I thought a class is not necessary for a one-off use case, but sure, can use a class instead

Yes, please use a class.

Can we convert this to a class instead of using an inline style

Sorry, i am a little late to the comment here. It is fine to have simple styles in JS like this considering we have css which is not well maintained. If we make this a class we most likely will never get to cleaning this part if the html changes

@devinbinnie, please confirm 🙂

@abdusabri Sure that's fine

devinbinnie · 2019-11-21T17:00:45Z

components/size_aware_image.jsx

+ src={src}
+ onError={this.handleError}
+ onLoad={this.handleLoad}
+ style={this.props.handleSmallImageContainer && this.state.isSmallImage ? {


Can this be done with class names instead of inline styles?

I would have loved to do it using a class, and actually tried multiple approaches before falling back to using in-line styles. There are multiple levels of inheritance and a specificity war is going on 🙈 🙃, and in such cases, I believe using in-line style is better than using !important.

~~It would be awesome and much appreciated If you could help with resolving the specificity issue without using !important.~~

Update: I've figured it out, resolved the specificity issue and moved it into a class instead of in-line style.

@abdusabri Thanks, but I'm not seeing the changes here

The change is done locally but not pushed yet. Sorry if my comment was confusing.

Okay no problem! :)

devinbinnie · 2019-11-21T17:01:04Z

components/size_aware_image.jsx

+ <div
+ onClick={this.props.onClick}
+ className={className}
+ style={this.state.imageWidth > MIN_IMAGE_SIZE ? {


Same here with respect to inline styles

This is using the actual image size after it has loaded, not a predictable/fixed value that can be set in a CSS class, and I don't know if there could be a practical way to use something like calc to set it in CSS either.

width: fit-content; works, but according to MDN docs, it is experimental and shouldn't be used in production code. And here are more details from can I use

Okay, we can leave this one.

devinbinnie · 2019-11-21T17:01:38Z

components/size_aware_image.jsx


 //The css hack here for loading images in the background can be removed after IE11 is dropped in 5.16v
 //We can go back to https://github.com/mattermost/mattermost-webapp/pull/2924/files
- if (!this.state.loaded && dimensions && dimensions.width) {
+ if (!this.state.loaded && this.dimensionsAvailable(dimensions)) {
 imageStyleChangesOnLoad = {position: 'absolute', top: 0, height: 1, width: 1, visibility: 'hidden', overflow: 'hidden'};


Can we move this to a class somewhere?

Not sure how best to handle this for a workaround - based on the code comment above this line -, I would love for @sudheerDev to chime in as he has more context about this

@devinbinnie, @sudheerDev I'm waiting for your feedback on this to complete the requested changes and push my local updates

Well I know IE11 is no longer supported by Mattermost, so it should be safe to remove altogether, @sudheerDev does that make sense?

In case it is gonna be removed altogether, is it better to do so in a separate PR? (so it is easier to isolate the change in case there are any regression issues because of it)

Either that or at least a separate commit.

A separate commit sounds good. I can do so once Sudheer confirms or provides his feedback.

@abdusabri @devinbinnie Yes, we can remove that as we don't need to support for IE11. IMO it should be a separate PR.

it is easier to isolate the change in case there are any regression issues because of it

I agree with this. i think we should isolate that change. We already have a lot in this PR already. Better not to make more changes on top of this. It is easier for git blame to track and understand why a change was made as well if it is in separate PR.

Ok, then let's do this in a separate PR.

Please feel free to create a ticket and assign it to me (@devinbinnie or @sudheerDev)

abdusabri · 2019-11-23T18:56:41Z

Thanks @devinbinnie for the feedback! 🙂

Please check my comments and let me know your thoughts, thanks!

andrewbrown00

@abdusabri good to go 😄

devinbinnie

Thanks @abdusabri! Can you sync the PR with master? Then we can merge it.

* Move small image styles when inside a container to a CSS class * Improve naming convention of MD inline img container CSS class * Update unit tests * Update and improve E2E tests

mattermod · 2019-12-03T17:51:22Z

New commit detected. SpinWick will upgrade if the updated docker image is available.

abdusabri · 2019-12-03T17:52:44Z

Thanks @abdusabri! Can you sync the PR with master? Then we can merge it.

Thanks @devinbinnie! Just pushed my local commits.

Since there were some updates, improvements, and a little merge conflict in E2E tests, it would be a nice bonus if @saturninoabril can have a quick look.

saturninoabril

@abdusabri Great to have those tests, well done!

saturninoabril · 2019-12-03T19:40:14Z

e2e/cypress/integration/channel/image_attachment_spec.js

+ cy.get('div.modal-image__content').should('be.visible');
+ });
+ });
+});


saturninoabril · 2019-12-03T19:40:27Z

e2e/cypress/integration/markdown/markdown_image_spec.js

+ should('have.class', 'markdown-inline-img--hover').
+ and('have.class', 'markdown-inline-img').
+ and('have.class', 'markdown-inline-img--scaled-down').
+ and('have.css', 'height', '18px');


mattermod · 2019-12-03T19:42:11Z

Test server destroyed

saturninoabril · 2019-12-04T09:48:42Z

/update-branch

Add a container to images below a min-width and min-height

a255df1

hanzei requested review from andrewbrown00 and esethna October 6, 2019 06:22

hanzei added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Oct 6, 2019

amyblais added the Hacktoberfest label Oct 7, 2019

andrewbrown00 suggested changes Oct 7, 2019

View reviewed changes

Fix stretched markdown image issue

93764f5

abdusabri added 3 commits October 8, 2019 23:25

Add hover effect for in-line MD image links, disable container handling

2344a31

Fix code indentation issue in markdown_image component

68588b4

Add hover effect for in-line MD images in channel header

bcba500

esethna suggested changes Oct 9, 2019

View reviewed changes

abdusabri added 2 commits October 10, 2019 23:42

Fix UX issues when markdown images are used in channel header

f68022f

Merge branch 'master' into MM-15232-add-container-to-small-images

dd6cdb3

abdusabri changed the title ~~[MM-15232] Add a container to images below a min-width and min-height to maintain clickable areas~~ [MM-15232] [MM-18158] Add a container to small images, Fix UX issues when markdown images are used in channel header Oct 10, 2019

abdusabri requested review from esethna and andrewbrown00 October 11, 2019 00:52

andrewbrown00 reviewed Oct 14, 2019

View reviewed changes

esethna removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Oct 18, 2019

cpanato requested a review from sudheerDev November 19, 2019 16:41

sudheerDev approved these changes Nov 21, 2019

View reviewed changes

devinbinnie suggested changes Nov 21, 2019

View reviewed changes

Merge branch 'master' into MM-15232-add-container-to-small-images

5d8c6be

andrewbrown00 approved these changes Dec 2, 2019

View reviewed changes

esethna added this to the v5.20.0 milestone Dec 3, 2019

devinbinnie self-requested a review December 3, 2019 14:25

devinbinnie approved these changes Dec 3, 2019

View reviewed changes

devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Dec 3, 2019

abdusabri added 3 commits December 3, 2019 18:19

Updates based on 2nd review comments

76444c7

* Move small image styles when inside a container to a CSS class * Improve naming convention of MD inline img container CSS class * Update unit tests * Update and improve E2E tests

Merge branch 'master' into MM-15232-add-container-to-small-images

98956b6

Update md image spec to revert an unintended change from auto-merge

3087197

saturninoabril approved these changes Dec 3, 2019

View reviewed changes

saturninoabril added QA Review Done and removed Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Dec 3, 2019

Merge branch 'master' into MM-15232-add-container-to-small-images

0f2bee9

saturninoabril merged commit 01dbe7b into mattermost:master Dec 4, 2019

This was referenced Dec 16, 2019

Fix inline markdown paragraph styles #4525

Merged

Fix incorrect link handling of inline MD images #4543

Merged

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 17, 2020

[MM-15232] [MM-18158] Add a container to small images, Fix UX issues when markdown images are used in channel header #3877

[MM-15232] [MM-18158] Add a container to small images, Fix UX issues when markdown images are used in channel header #3877

Conversation

abdusabri commented Oct 6, 2019 • edited Loading

Summary

Ticket Link

andrewbrown00 left a comment

Choose a reason for hiding this comment

abdusabri commented Oct 8, 2019

esethna commented Oct 8, 2019 • edited Loading

abdusabri commented Oct 8, 2019 • edited Loading

andrewbrown00 commented Oct 8, 2019

abdusabri commented Oct 8, 2019

esethna left a comment • edited Loading

Choose a reason for hiding this comment

abdusabri commented Oct 9, 2019

lieut-data commented Oct 11, 2019

andrewbrown00 left a comment • edited Loading

Choose a reason for hiding this comment

abdusabri commented Oct 14, 2019

andrewbrown00 commented Oct 14, 2019 • edited Loading

abdusabri commented Oct 14, 2019

sudheerDev commented Nov 20, 2019 • edited Loading

abdusabri commented Nov 21, 2019

sudheerDev left a comment

Choose a reason for hiding this comment

sudheerDev commented Nov 21, 2019

devinbinnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdusabri Nov 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdusabri Nov 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdusabri Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdusabri commented Nov 23, 2019

andrewbrown00 left a comment

Choose a reason for hiding this comment

devinbinnie left a comment • edited Loading

Choose a reason for hiding this comment

mattermod commented Dec 3, 2019

abdusabri commented Dec 3, 2019 • edited Loading

saturninoabril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermod commented Dec 3, 2019

saturninoabril commented Dec 4, 2019

abdusabri commented Oct 6, 2019 •

edited

Loading

esethna commented Oct 8, 2019 •

edited

Loading

abdusabri commented Oct 8, 2019 •

edited

Loading

esethna left a comment •

edited

Loading

andrewbrown00 left a comment •

edited

Loading

andrewbrown00 commented Oct 14, 2019 •

edited

Loading

sudheerDev commented Nov 20, 2019 •

edited

Loading

abdusabri Nov 23, 2019 •

edited

Loading

abdusabri Nov 23, 2019 •

edited

Loading

abdusabri Dec 2, 2019 •

edited

Loading

devinbinnie left a comment •

edited

Loading

abdusabri commented Dec 3, 2019 •

edited

Loading