-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@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:
As far as I understand, this is case Thanks! 🙂 |
@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 |
@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 |
@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 😄 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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):
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. |
/check-cla |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdusabri thanks for the updates, a few small changes listed below:
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
@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? |
My apologies @abdusabri I saw this 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. |
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? |
@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 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!. Thanks @abdusabri
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just some small changes around CSS requested. Thanks @abdusabri!
alt={alt} | ||
src={brokenImageIcon} | ||
/> | ||
<div style={{display: 'inline-block'}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we convert this to a class instead of using an inline style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought a class is not necessary for a one-off use case, but sure, can use a class instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please use a class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devinbinnie, please confirm 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdusabri Sure that's fine
components/size_aware_image.jsx
Outdated
src={src} | ||
onError={this.handleError} | ||
onLoad={this.handleLoad} | ||
style={this.props.handleSmallImageContainer && this.state.isSmallImage ? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done with class names instead of inline styles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdusabri Thanks, but I'm not seeing the changes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is done locally but not pushed yet. Sorry if my comment was confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay no problem! :)
<div | ||
onClick={this.props.onClick} | ||
className={className} | ||
style={this.state.imageWidth > MIN_IMAGE_SIZE ? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here with respect to inline styles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, we can leave this one.
|
||
//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'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to a class somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devinbinnie, @sudheerDev I'm waiting for your feedback on this to complete the requested changes and push my local updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I know IE11 is no longer supported by Mattermost, so it should be safe to remove altogether, @sudheerDev does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either that or at least a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate commit sounds good. I can do so once Sudheer confirms or provides his feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdusabri @devinbinnie Yes, we can remove that as we don't need to support for IE11. IMO it should be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Thanks @devinbinnie for the feedback! 🙂 Please check my comments and let me know your thoughts, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdusabri good to go 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
New commit detected. SpinWick will upgrade if the updated docker image is available. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdusabri Great to have those tests, well done!
cy.get('div.modal-image__content').should('be.visible'); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Test server destroyed |
/update-branch |
Summary
Attached images
and in-line markdown imageswith a width or height below 48px will be placed inside a container div to allow for better click/tap targeting.Expected behavior:
- 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:
Screenshots
Ticket Link
Fixes mattermost/mattermost#12187 and https://mattermost.atlassian.net/browse/MM-18158