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

MM-13099 Use size_aware_image component for when loading images in posts #2334

Merged
merged 5 commits into from
Mar 8, 2019
Merged

MM-13099 Use size_aware_image component for when loading images in posts #2334

merged 5 commits into from
Mar 8, 2019

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented Feb 5, 2019 •
edited
Loading

Summary

  • Removing getFileDimensionsForDisplay in favour of SizeAwareImage as it would be reliable in
    preventing scroll pop
  • Changes also includes standard dimensions for svg as we don't have dimensions for old svgs

Ticket Link

MM-13099
MM-13158

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Has UI changes

@sudheerDev sudheerDev added the Work in Progress Not yet ready for review label Feb 5, 2019
@sudheerDev
Copy link
Contributor Author

@asaadmahmood We have an issue with svg uploads and scroll pop.
With images in posts we get dimensions from server and we use that to create placeholders and load image inside this placeholder to avoid scroll pop. but with svg we cannot get dimensions which can cause scroll pop

One solution we thought is to have a fixed height and let the width scale to keep the aspect ratio.

screenshot 2019-02-05 at 3 28 03 pm

Ex: The above svg has 150px fixed height

The other solution is to show it as attachments instead of preview for images as most platforms do to avoid this problem.

Let me know if you have any suggestions or solutions for fixing this.

@asaadmahmood
Copy link
Contributor

@sudheerDev Maintaining a fixed height solution for the svgs make sense, we can keep the max height at 150px, or whatever the maximum height is to avoid a scrollpop.

@sudheerDev sudheerDev added the Setup Old Test Server Triggers the creation of a test server label Feb 5, 2019
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@mattermod
Copy link
Contributor

Spinmint test server created at: https://i-0ba2cdcfd45b19a72.test.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-0ba2cdcfd45b19a72

@sudheerDev sudheerDev added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed Setup Old Test Server Triggers the creation of a test server Work in Progress Not yet ready for review labels Feb 21, 2019
@sudheerDev
Copy link
Contributor Author

Waiting for a fix that will be on server end to clean up svg issue here.

@sudheerDev sudheerDev mentioned this pull request Mar 5, 2019
Merged
31 tasks
@sudheerDev
MM-13099 Use size_aware_image component for all instances for loading…
cedd5df 
… images in webapp.

* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
@sudheerDev
WIP changes for svg pop
97bc934
@sudheerDev
Fix snapshots
29cf7fb
@sudheerDev sudheerDev added 2: Dev Review Requires review by a core commiter and removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Mar 7, 2019
@sudheerDev sudheerDev added this to the v5.10.0 milestone Mar 7, 2019
enahum
enahum approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

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

nice!

saturninoabril
saturninoabril approved these changes Mar 8, 2019
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

💯

@saturninoabril saturninoabril merged commit 52ef9b9 into mattermost:master Mar 8, 2019
@sudheerDev sudheerDev deleted the MM-13099 branch March 8, 2019 07:57
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Mar 11, 2019
@sudheerDev
MM-13099 Use size_aware_image component for when loading images in po…
baa8bdf 
…sts (mattermost#2334)

* MM-13099 Use size_aware_image component for all instances for loading images in webapp.
* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
* Add a fallback for svg as older version do not have dimentions
MM-14500 Fix for pop caused by size_aware_image

  * Use svg for creating placeholder instead of canvas and dataUrl
    Change to inline-block for hover css shadow effect
    Add defensive check for dimensions

Removing display: inline-flex as it causes svg in the placeholder to not take the available space
Have to move border hioghlight to img tags as div is display block by default so shows border for the entire
box instead of just the image

Update snapshots and fix lint
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Mar 12, 2019
@sudheerDev
MM-13099 Use size_aware_image component for when loading images in po…
a25af9e 
…sts (mattermost#2334)

* MM-13099 Use size_aware_image component for all instances for loading images in webapp.
* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
* Add a fallback for svg as older version do not have dimentions
MM-14500 Fix for pop caused by size_aware_image

  * Use svg for creating placeholder instead of canvas and dataUrl
    Change to inline-block for hover css shadow effect
    Add defensive check for dimensions

Removing display: inline-flex as it causes svg in the placeholder to not take the available space
Have to move border hioghlight to img tags as div is display block by default so shows border for the entire
box instead of just the image

Update snapshots and fix lint
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Mar 14, 2019
@sudheerDev
MM-13099 Use size_aware_image component for when loading images in po…
e064730 
…sts (mattermost#2334)

* MM-13099 Use size_aware_image component for all instances for loading images in webapp.
* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
* Add a fallback for svg as older version do not have dimentions
MM-14500 Fix for pop caused by size_aware_image

  * Use svg for creating placeholder instead of canvas and dataUrl
    Change to inline-block for hover css shadow effect
    Add defensive check for dimensions

Removing display: inline-flex as it causes svg in the placeholder to not take the available space
Have to move border hioghlight to img tags as div is display block by default so shows border for the entire
box instead of just the image

Update snapshots and fix lint
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 19, 2019
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Mar 19, 2019
@sudheerDev
MM-13099 Use size_aware_image component for when loading images in po…
7453b0e 
…sts (mattermost#2334)

* MM-13099 Use size_aware_image component for all instances for loading images in webapp.
* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
* Add a fallback for svg as older version do not have dimentions
MM-14500 Fix for pop caused by size_aware_image

  * Use svg for creating placeholder instead of canvas and dataUrl
    Change to inline-block for hover css shadow effect
    Add defensive check for dimensions

Removing display: inline-flex as it causes svg in the placeholder to not take the available space
Have to move border hioghlight to img tags as div is display block by default so shows border for the entire
box instead of just the image

Update snapshots and fix lint
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Mar 20, 2019
@sudheerDev
MM-13099 Use size_aware_image component for when loading images in po…
3434845 
…sts (mattermost#2334)

* MM-13099 Use size_aware_image component for all instances for loading images in webapp.
* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
* Add a fallback for svg as older version do not have dimentions
MM-14500 Fix for pop caused by size_aware_image

  * Use svg for creating placeholder instead of canvas and dataUrl
    Change to inline-block for hover css shadow effect
    Add defensive check for dimensions

Removing display: inline-flex as it causes svg in the placeholder to not take the available space
Have to move border hioghlight to img tags as div is display block by default so shows border for the entire
box instead of just the image

Update snapshots and fix lint
hmhealey pushed a commit that referenced this pull request Mar 20, 2019
@sudheerDev @hmhealey
MM-14500 Fix for pop caused by size_aware_image (#2469)
ee41ff2 
* MM-13099 Use size_aware_image component for when loading images in posts (#2334)
* MM-13099 Use size_aware_image component for all instances for loading images in webapp.
* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
* Add a fallback for svg as older version do not have dimentions
MM-14500 Fix for pop caused by size_aware_image

  * Use svg for creating placeholder instead of canvas and dataUrl
    Change to inline-block for hover css shadow effect
    Add defensive check for dimensions

Removing display: inline-flex as it causes svg in the placeholder to not take the available space
Have to move border hioghlight to img tags as div is display block by default so shows border for the entire
box instead of just the image

Update snapshots and fix lint

* Fix review comments
stevepartridge pushed a commit to stevepartridge/mattermost-webapp that referenced this pull request Mar 30, 2019
@sudheerDev @stevepartridge
MM-13099 Use size_aware_image component for when loading images in po…
d1604b4 
…sts (mattermost#2334)

* MM-13099 Use size_aware_image component for all instances for loading images in webapp.

* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop

* WIP changes for svg pop

* Fix snapshots

* Add a fallback for svg as older version do not have dimentions

* Revert package-lock change
stevepartridge pushed a commit to stevepartridge/mattermost-webapp that referenced this pull request Mar 30, 2019
@sudheerDev @stevepartridge
MM-14500 Fix for pop caused by size_aware_image (mattermost#2469)
d1a1784 
* MM-13099 Use size_aware_image component for when loading images in posts (mattermost#2334)
* MM-13099 Use size_aware_image component for all instances for loading images in webapp.
* Removing getFileDimensionsForDisplay in favour of  SizeAwareImage as it would be reliable in preventing
  scroll pop
* Add a fallback for svg as older version do not have dimentions
MM-14500 Fix for pop caused by size_aware_image

  * Use svg for creating placeholder instead of canvas and dataUrl
    Change to inline-block for hover css shadow effect
    Add defensive check for dimensions

Removing display: inline-flex as it causes svg in the placeholder to not take the available space
Have to move border hioghlight to img tags as div is display block by default so shows border for the entire
box instead of just the image

Update snapshots and fix lint

* Fix review comments
@DHaussermann DHaussermann added the Tests/Done Release tests have been written label Apr 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@saturninoabril saturninoabril saturninoabril approved these changes

@enahum enahum enahum approved these changes

Assignees
No one assigned
Labels
2: Dev Review Requires review by a core commiter Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.10.0
Development

Successfully merging this pull request may close these issues.
7 participants
@sudheerDev @asaadmahmood @mattermod @saturninoabril @enahum @amyblais @DHaussermann