[MM-12290] inline image collapse #7352

TheDarkestDay · 2021-01-20T09:57:25Z

Summary

The purpose of this PR is to introduce capability for inline Markdown image to be expanded/collapsed. Moreover, they should take into account user preferences in terms of default embedded content display mode - either collapsed or expanded.

Ticket Link

mattermost/mattermost#16636

Related Pull Requests

N/A

Screenshots

Hovering large inline image now displays a special "Collapse" button in the top-left corner of the image:

You can collapse it by clicking on the button and then expand it again by clicking on a special collapsed view of the inline image:

…design

…webapp into MM-12290-inline-image-collapse

… MarkdownImage, added handling for both imageMetadata.height and regular height for MarkdownImageExpand

mattermod · 2021-01-20T09:57:29Z

Hello @TheDarkestDay,

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.

matthewbirtch

Thanks @TheDarkestDay! This is great. Just a few tweaks from my standpoint:

The image has a border on it when you hover on it. Can we remove that?
Is it possible to animate the collapsing transition? This would help orient users. If not, we could look at that in a separate PR
Can we reduce the margin on the top of .markdown-inline-img to 2px instead of 5px? That should even out the positioning of the arrow button

matthewbirtch · 2021-01-21T14:41:04Z

components/markdown_image_expand/markdown_image_expand.scss

+ &:hover {
+ border: 1px solid var(--center-channel-color-24);
+
+ @include box-shadow($elevation-2);


For some reason $elevation-2 is rendering the shadow opacity at 0.012 and it should be rendering as 0.12

Alright. I'll fix this. By the way, all of the elevations starting from 2 to 6 have an opacity like this. Probably we have here some sort of wide spread issue.

Seems like a mistake to me. Not sure when this was added

Fixed only $elevation-2 for now.

@asaadmahmood I am sorry to bother you but according to Git blame you were the author of these elevation settings. May I request some assistance from you - are these opacity values incorrect or there were some reasoning behind this?

@TheDarkestDay The opacity values are updated on master.
I did them on this PR:
https://github.com/mattermost/mattermost-webapp/pull/7696/files

TheDarkestDay · 2021-01-25T19:03:41Z

Thanks @TheDarkestDay! This is great. Just a few tweaks from my standpoint:

The image has a border on it when you hover on it. Can we remove that?

Is it possible to animate the collapsing transition? This would help orient users. If not, we could look at that in a separate PR

Can we reduce the margin on the top of .markdown-inline-img to 2px instead of 5px? That should even out the positioning of the arrow button

Thanks @matthewbirtch! Okay, let me try to tackle this step by step.

I've been digging around point 3 and the only question I am currently having - should we apply this change universally to all of the Markdown inline images or should just create some special edge case for this one?

I tried applying this thing globally and I didn't manage to find any specific issues caused by this change. On the other hand, for sure I might be missing some use case which would really explain the purpose of this margin setting.

What do you think, does it make sense to apply this change universally, push it here and let you and the team check it?

TheDarkestDay · 2021-01-25T19:07:37Z

The image has a border on it when you hover on it. Can we remove that?

Oh, that's the same story, actually.

The thing is that each inline Markdown image is supposed to be clickable - and on click it should be shown in full size in a modal window. And I guess the reason for this shadow-on-hover effect is exactly to give a hint that this image is clickable.

So, the question is - should we treat these "images with expand" somehow different? Should we remove this thing for them or rework somehow?

TheDarkestDay · 2021-01-25T19:10:40Z

Is it possible to animate the collapsing transition? This would help orient users. If not, we could look at that in a separate PR

I think at least I can try. The only thing I am concerned about here is performance of height change animation - but we will only find it out if we try it in the wild.

May I kindly ask you to share some assets for this animation? Things like duration, easing function, maybe even some gif would be helpful.

matthewbirtch · 2021-01-25T19:21:09Z

I've been digging around point 3 and the only question I am currently having - should we apply this change universally to all of the Markdown inline images or should just create some special edge case for this one?

I tried applying this thing globally and I didn't manage to find any specific issues caused by this change. On the other hand, for sure I might be missing some use case which would really explain the purpose of this margin setting.

What do you think, does it make sense to apply this change universally, push it here and let you and the team check it?

Ok, let's leave this as-is for now and we can look in to it later if we feel strongly that it needs to change.

The thing is that each inline Markdown image is supposed to be clickable - and on click it should be shown in full size in a modal window. And I guess the reason for this shadow-on-hover effect is exactly to give a hint that this image is clickable.

So, the question is - should we treat these "images with expand" somehow different? Should we remove this thing for them or rework somehow?

Makes sense. Let's leave the border and shadow effect

I think at least I can try. The only thing I am concerned about here is performance of height change animation - but we will only find it out if we try it in the wild.

May I kindly ask you to share some assets for this animation? Things like duration, easing function, maybe even some gif would be helpful.

Give these a go:

transition: height 250ms cubic-bezier(0.5, 0, 0.5, 1);

We may need to tweak the above, but we'll see when it's in the wild.

…webapp into MM-12290-inline-image-collapse

jgilliam17

Thank you @TheDarkestDay
Tested, looks good to merge.

Verified inline image collapse for images 100px or more in height; heights change animations when using slash commands for expand/collapse are happening in view; underline is removed when inline image is linked.

mm-cloud-bot · 2021-04-07T14:04:01Z

Test server destroyed

jgilliam17 · 2021-04-07T14:08:57Z

@hmhealey Can you please help merge. Thanks 🙂

hmhealey · 2021-04-08T13:42:34Z

Merged! Thanks again for the PR, @TheDarkestDay. This will be on Mattermost Cloud some time in the next couple weeks and then in (I think) Mattermost 5.35 after that.

ethervoid · 2021-04-12T10:36:24Z

/cherry-pick cloud

mattermod · 2021-04-12T10:36:25Z

Cherry pick is scheduled.

* MM-12290. More or less done with the concept, visuals and components design * MM-12290. Added hover styles for both collapse and expand icons * MM-12290. Changed hover style for expand button * MM-12290. Added snapshot tests for MarkdownImageExpand * MM-12290. Integrated proper icons for both expand and collapse buttons * MM-12290. Added corresponding tests for Expand insertion logic inside MarkdownImage, added handling for both imageMetadata.height and regular height for MarkdownImageExpand * MM-12290. Formatting fixes * MM-12290. Fixed snapshots for MarkdownImageExpand * MM-12290. An attempt to implement simple expand/collapse transition * MM-12290. Fixed wrong shadow props for elevation-2 * MM-12290. Auto-formatting * MM-12290. Inline image expand now properly captures settings values changed via slash commands like /expand or /collapse. Therefore, postId is now also provided for MarkdownImageExpand * [MM-12290] Got rid of local expand state in favor of Redux one, removed animations for now * [MM-12290] Moved inline image collapse/expand state to the corresponding storage field * [MM-12290] Fixed snapshots for html-to-component * [MM-12290] Fixed color usage to use setting opacity with rgba instead of using variables with opacities * [MM-12290] Reverted package-lock fully * [MM-12290] Missing newlinew for package-lock added back * [MM-12290] Minor codestyle fixes, now we use image src as its expand/collapse state key instead of index * [MM-12290] Added separate callback to let MarkdownImage notify consumers about some inner height change * [MOB-12290] Reverted package-lock changes, fixed missing heading for MarkdiwnImage test, fixed snapshots * [MM-12290] Written back a missing snapshot for fixed test * [MM-12290] Hopefully finally removed obsolete snapshot * [MM-12290] Removed redundant license information from MarkdownImageExpand * [MM-12290] Tried migrating expand-button display to inline-block to avoid displaying underline text decoration caused by parent link * [MM-12290] Fixed linting errors Co-authored-by: Aleksandr Brenchev <[email protected]> Co-authored-by: Mattermod <[email protected]> (cherry picked from commit 66b49a4)

* MM-12290. More or less done with the concept, visuals and components design * MM-12290. Added hover styles for both collapse and expand icons * MM-12290. Changed hover style for expand button * MM-12290. Added snapshot tests for MarkdownImageExpand * MM-12290. Integrated proper icons for both expand and collapse buttons * MM-12290. Added corresponding tests for Expand insertion logic inside MarkdownImage, added handling for both imageMetadata.height and regular height for MarkdownImageExpand * MM-12290. Formatting fixes * MM-12290. Fixed snapshots for MarkdownImageExpand * MM-12290. An attempt to implement simple expand/collapse transition * MM-12290. Fixed wrong shadow props for elevation-2 * MM-12290. Auto-formatting * MM-12290. Inline image expand now properly captures settings values changed via slash commands like /expand or /collapse. Therefore, postId is now also provided for MarkdownImageExpand * [MM-12290] Got rid of local expand state in favor of Redux one, removed animations for now * [MM-12290] Moved inline image collapse/expand state to the corresponding storage field * [MM-12290] Fixed snapshots for html-to-component * [MM-12290] Fixed color usage to use setting opacity with rgba instead of using variables with opacities * [MM-12290] Reverted package-lock fully * [MM-12290] Missing newlinew for package-lock added back * [MM-12290] Minor codestyle fixes, now we use image src as its expand/collapse state key instead of index * [MM-12290] Added separate callback to let MarkdownImage notify consumers about some inner height change * [MOB-12290] Reverted package-lock changes, fixed missing heading for MarkdiwnImage test, fixed snapshots * [MM-12290] Written back a missing snapshot for fixed test * [MM-12290] Hopefully finally removed obsolete snapshot * [MM-12290] Removed redundant license information from MarkdownImageExpand * [MM-12290] Tried migrating expand-button display to inline-block to avoid displaying underline text decoration caused by parent link * [MM-12290] Fixed linting errors Co-authored-by: Aleksandr Brenchev <[email protected]> Co-authored-by: Mattermod <[email protected]> (cherry picked from commit 66b49a4) Co-authored-by: Alexander Brenchev <[email protected]>

Documentation for: mattermost/mattermost-webapp#7352 Updated: - User's Guide > Messaging > Formatting Text > In-line Images - Added introduction to explain what in-line images are and clarify that in-line images over 100px in height are subject to the **Default Appearance of Image Previews** user preference and the equivalent slash commands - Applied readability and consistency updates to existing content - Corrected formatting where needed

Documentation for: mattermost/mattermost-webapp#7352 Updated: - User's Guide > Settings > Account Settings > Display > Link Previews - Updated the existing subsection title from "Link Previews" to "Default Appearance of Image Previews" - Updated impact scope of user preference to include in-line images over 100px in height

Documentation for: mattermost/mattermost-webapp#7352 Updated: - Cloud Admin Guide > Configuration > Site Configuration - Applied Style Guide, consistency, and navigation updates to content throughout this page - Under **Posts**, added documentation for users to control whether in-line images over 100px in height can be automatically collapsed or expanded

Documentation for: mattermost/mattermost-webapp#7352 Updated: - Cloud Admin Guide > Integrations > Slash Commands > Built-in Commands - Updated descriptions for both ``/expand`` and ``/collapse`` to include image previews, image attachments, and inline images over 100px in height, and included links to the in-line images user documentation

Documentation for: mattermost/mattermost-webapp#7352 Updated: - User's Guide > Messaging > Executing Commands > Built-in Commands - Updated descriptions for both ``/expand`` and ``/collapse`` to include image previews, image attachments, and inline images over 100px in height, and included links to the in-line images user documentation

* MM-12290 inline image collapse Documentation for: mattermost/mattermost-webapp#7352 Updated: - User's Guide > Messaging > Formatting Text > In-line Images - Added introduction to explain what in-line images are and clarify that in-line images over 100px in height are subject to the **Default Appearance of Image Previews** user preference and the equivalent slash commands - Applied readability and consistency updates to existing content - Corrected formatting where needed * MM-12290 inline image collapse Documentation for: mattermost/mattermost-webapp#7352 Updated: - User's Guide > Settings > Account Settings > Display > Link Previews - Updated the existing subsection title from "Link Previews" to "Default Appearance of Image Previews" - Updated impact scope of user preference to include in-line images over 100px in height * Update source/help/settings/account-settings.rst * Update source/help/messaging/formatting-text.rst * MM-12290 inline image collapse Documentation for: mattermost/mattermost-webapp#7352 Updated: - Cloud Admin Guide > Configuration > Site Configuration - Applied Style Guide, consistency, and navigation updates to content throughout this page - Under **Posts**, added documentation for users to control whether in-line images over 100px in height can be automatically collapsed or expanded * Update source/help/messaging/formatting-text.rst * Update source/cloud/cloud-administration/site-configuration.rst * Update source/cloud/cloud-administration/site-configuration.rst * MM-12290 inline image collapse Documentation for: mattermost/mattermost-webapp#7352 Updated: - Cloud Admin Guide > Integrations > Slash Commands > Built-in Commands - Updated descriptions for both ``/expand`` and ``/collapse`` to include image previews, image attachments, and inline images over 100px in height, and included links to the in-line images user documentation * MM-12290 inline image collapse Documentation for: mattermost/mattermost-webapp#7352 Updated: - User's Guide > Messaging > Executing Commands > Built-in Commands - Updated descriptions for both ``/expand`` and ``/collapse`` to include image previews, image attachments, and inline images over 100px in height, and included links to the in-line images user documentation * Formatting fixes * Alignment fixes * Changed Note to Tip * Update source/cloud/cloud-administration/site-configuration.rst * MM-12290 inline image collapse Applied reviewer feedback * MM-12290 inline image collapse Applied reviewer feedback * MM-12290 inline image collapse Applied reviewer feedback. * MM-12290 inline image collapse Applied reviewer feedback. * MM-12290-inline-image-collapse Formatting fixes * MM-12290 inline image collapse Formatting fixes * MM-12290 inline image collapse Formatting fixes * formatting fixes * Update source/cloud/cloud-administration/site-configuration.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/cloud/cloud-administration/site-configuration.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/cloud/cloud-administration/site-configuration.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/cloud/cloud-administration/site-configuration.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/help/messaging/formatting-text.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/help/messaging/formatting-text.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/help/messaging/formatting-text.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/help/messaging/formatting-text.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/help/messaging/formatting-text.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/cloud/cloud-administration/site-configuration.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/cloud/cloud-administration/site-configuration.rst Co-authored-by: Justine Geffen <[email protected]> * Update source/cloud/cloud-administration/site-configuration.rst Co-authored-by: Justine Geffen <[email protected]> Co-authored-by: Justine Geffen <[email protected]>

mm-cloud-bot · 2021-04-21T18:22:10Z

@TheDarkestDay: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

Aleksandr Brenchev added 9 commits January 13, 2021 20:20

MM-12290. More or less done with the concept, visuals and components …

9656716

…design

MM-12290. Added hover styles for both collapse and expand icons

cd13249

MM-12290. Changed hover style for expand button

6673f11

MM-12290. Added snapshot tests for MarkdownImageExpand

fc19cd2

Merge branch 'master' of https://github.com/TheDarkestDay/mattermost-…

7dbefb3

…webapp into MM-12290-inline-image-collapse

MM-12290. Integrated proper icons for both expand and collapse buttons

86cac4b

MM-12290. Added corresponding tests for Expand insertion logic inside…

6cb903c

… MarkdownImage, added handling for both imageMetadata.height and regular height for MarkdownImageExpand

MM-12290. Formatting fixes

527d238

MM-12290. Fixed snapshots for MarkdownImageExpand

877412b

TheDarkestDay mentioned this pull request Jan 20, 2021

Allow collapsing in-line images over 100px in height mattermost/mattermost#16636

Closed

matthewbirtch self-requested a review January 20, 2021 14:57

matthewbirtch added 1: PM Review Requires review by a product manager 1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 20, 2021

hanzei added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jan 20, 2021

matthewbirtch suggested changes Jan 21, 2021

View reviewed changes

matthewbirtch requested review from esethna, hmhealey and jgilliam17 January 21, 2021 14:56

jgilliam17 changed the title ~~Mm 12290 inline image collapse~~ [MM-12290] inline image collapse Jan 21, 2021

Aleksandr Brenchev added 4 commits January 27, 2021 11:00

MM-12290. An attempt to implement simple expand/collapse transition

84003b6

MM-12290. Fixed wrong shadow props for elevation-2

cc28164

Merge branch 'master' of https://github.com/TheDarkestDay/mattermost-…

ce24710

…webapp into MM-12290-inline-image-collapse

MM-12290. Auto-formatting

775b57a

jgilliam17 approved these changes Apr 7, 2021

View reviewed changes

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 Apr 7, 2021

hmhealey merged commit 66b49a4 into mattermost:master Apr 8, 2021

amyblais added this to the v5.35 milestone Apr 8, 2021

amyblais added Changelog/Done Required changelog entry has been written Docs/Needed Requires documentation labels Apr 8, 2021

mattermost-build mentioned this pull request Apr 12, 2021

Automated cherry pick of #7352 #7882

Merged

mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Apr 12, 2021

cwarnermm mentioned this pull request Apr 16, 2021

MM-12290 inline image collapse mattermost/docs#4495

Merged

cwarnermm added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Apr 21, 2021

mm-cloud-bot added the do-not-merge/release-note-label-needed label Apr 21, 2021

wiersgallak mentioned this pull request Jun 8, 2021

Markup doesn't take into account image preview settings mattermost/mattermost#9451

Closed

tdwio mentioned this pull request Apr 30, 2024

Fix collapsible markdown images mattermost/mattermost#26905

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-12290] inline image collapse #7352

[MM-12290] inline image collapse #7352

TheDarkestDay commented Jan 20, 2021

mattermod commented Jan 20, 2021

matthewbirtch left a comment

matthewbirtch Jan 21, 2021

TheDarkestDay Jan 25, 2021

matthewbirtch Jan 25, 2021

TheDarkestDay Jan 27, 2021

asaadmahmood Mar 17, 2021

TheDarkestDay commented Jan 25, 2021

TheDarkestDay commented Jan 25, 2021

TheDarkestDay commented Jan 25, 2021 •

edited

Loading

matthewbirtch commented Jan 25, 2021 •

edited

Loading

jgilliam17 left a comment

mm-cloud-bot commented Apr 7, 2021

jgilliam17 commented Apr 7, 2021

hmhealey commented Apr 8, 2021

ethervoid commented Apr 12, 2021

mattermod commented Apr 12, 2021

mm-cloud-bot commented Apr 21, 2021

[MM-12290] inline image collapse #7352

[MM-12290] inline image collapse #7352

Conversation

TheDarkestDay commented Jan 20, 2021

Summary

Ticket Link

Related Pull Requests

Screenshots

mattermod commented Jan 20, 2021

matthewbirtch left a comment

Choose a reason for hiding this comment

matthewbirtch Jan 21, 2021

Choose a reason for hiding this comment

TheDarkestDay Jan 25, 2021

Choose a reason for hiding this comment

matthewbirtch Jan 25, 2021

Choose a reason for hiding this comment

TheDarkestDay Jan 27, 2021

Choose a reason for hiding this comment

asaadmahmood Mar 17, 2021

Choose a reason for hiding this comment

TheDarkestDay commented Jan 25, 2021

TheDarkestDay commented Jan 25, 2021

TheDarkestDay commented Jan 25, 2021 • edited Loading

matthewbirtch commented Jan 25, 2021 • edited Loading

jgilliam17 left a comment

Choose a reason for hiding this comment

mm-cloud-bot commented Apr 7, 2021

jgilliam17 commented Apr 7, 2021

hmhealey commented Apr 8, 2021

ethervoid commented Apr 12, 2021

mattermod commented Apr 12, 2021

mm-cloud-bot commented Apr 21, 2021

TheDarkestDay commented Jan 25, 2021 •

edited

Loading

matthewbirtch commented Jan 25, 2021 •

edited

Loading