From 34348455e49b8c1a7ad323b52d977d2be08f09e1 Mon Sep 17 00:00:00 2001 From: Sudheer Date: Fri, 8 Mar 2019 13:26:19 +0530 Subject: [PATCH 1/2] 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 --- .../size_aware_image.test.jsx.snap | 20 +- .../message_attachment.test.jsx.snap | 12 +- .../message_attachment/message_attachment.jsx | 10 +- .../post_attachment_opengraph.test.js.snap | 187 ++++++++++++++++++ .../post_attachment_opengraph.jsx | 115 ++++------- .../post_attachment_opengraph.test.js | 123 ++++++++++++ .../post_view/post_image/post_image.jsx | 2 +- .../post_message_view.test.jsx.snap | 10 +- .../post_message_view/post_message_view.jsx | 2 +- .../single_image_view.test.jsx.snap | 158 ++++++++++----- .../single_image_view/single_image_view.jsx | 160 +++++---------- .../single_image_view.test.jsx | 36 ++-- components/size_aware_image.jsx | 74 ++++--- components/size_aware_image.test.jsx | 22 +-- .../__snapshots__/post_type.test.jsx.snap | 2 +- sass/components/_files.scss | 35 ++-- utils/file_utils.jsx | 24 --- utils/file_utils.test.jsx | 33 ---- utils/image_utils.jsx | 16 -- 19 files changed, 627 insertions(+), 414 deletions(-) create mode 100644 components/post_view/post_attachment_opengraph/__snapshots__/post_attachment_opengraph.test.js.snap create mode 100644 components/post_view/post_attachment_opengraph/post_attachment_opengraph.test.js diff --git a/components/__snapshots__/size_aware_image.test.jsx.snap b/components/__snapshots__/size_aware_image.test.jsx.snap index 7882b3487ee8..7405f6712b22 100644 --- a/components/__snapshots__/size_aware_image.test.jsx.snap +++ b/components/__snapshots__/size_aware_image.test.jsx.snap @@ -13,8 +13,22 @@ exports[`components/SizeAwareImage should render a placeholder and has loader wh containerClass="file__image-loading" /> - +
+
+ +
+
`; diff --git a/components/post_view/message_attachments/message_attachment/__snapshots__/message_attachment.test.jsx.snap b/components/post_view/message_attachments/message_attachment/__snapshots__/message_attachment.test.jsx.snap index 112eb81e113a..4e5f44832e8e 100644 --- a/components/post_view/message_attachments/message_attachment/__snapshots__/message_attachment.test.jsx.snap +++ b/components/post_view/message_attachments/message_attachment/__snapshots__/message_attachment.test.jsx.snap @@ -67,7 +67,7 @@ exports[`components/post_view/MessageAttachment should call actions.doPostAction @@ -114,7 +114,7 @@ exports[`components/post_view/MessageAttachment should call actions.doPostAction "width": 200, } } - onHeightReceived={[Function]} + onImageLoaded={[Function]} src="thumb_url" /> @@ -198,7 +198,7 @@ exports[`components/post_view/MessageAttachment should match snapshot 1`] = ` @@ -230,7 +230,7 @@ exports[`components/post_view/MessageAttachment should match snapshot 1`] = ` "width": 200, } } - onHeightReceived={[Function]} + onImageLoaded={[Function]} src="thumb_url" /> diff --git a/components/post_view/message_attachments/message_attachment/message_attachment.jsx b/components/post_view/message_attachments/message_attachment/message_attachment.jsx index d6c7c944eac3..8f3d28390137 100644 --- a/components/post_view/message_attachments/message_attachment/message_attachment.jsx +++ b/components/post_view/message_attachments/message_attachment/message_attachment.jsx @@ -60,7 +60,7 @@ export default class MessageAttachment extends React.PureComponent { }; this.imageProps = { - onHeightReceived: this.handleHeightReceived, + onImageLoaded: this.handleHeightReceived, }; } @@ -72,14 +72,14 @@ export default class MessageAttachment extends React.PureComponent { this.mounted = false; } - handleHeightReceivedForThumbUrl = (height) => { + handleHeightReceivedForThumbUrl = ({height}) => { const {attachment} = this.props; if (!this.props.imagesMetadata || (this.props.imagesMetadata && !this.props.imagesMetadata[attachment.thumb_url])) { this.handleHeightReceived(height); } } - handleHeightReceivedForImageUrl = (height) => { + handleHeightReceivedForImageUrl = ({height}) => { const {attachment} = this.props; if (!this.props.imagesMetadata || (this.props.imagesMetadata && !this.props.imagesMetadata[attachment.image_url])) { this.handleHeightReceived(height); @@ -347,7 +347,7 @@ export default class MessageAttachment extends React.PureComponent {
@@ -360,7 +360,7 @@ export default class MessageAttachment extends React.PureComponent { thumb = (
diff --git a/components/post_view/post_attachment_opengraph/__snapshots__/post_attachment_opengraph.test.js.snap b/components/post_view/post_attachment_opengraph/__snapshots__/post_attachment_opengraph.test.js.snap new file mode 100644 index 000000000000..61760a7dfada --- /dev/null +++ b/components/post_view/post_attachment_opengraph/__snapshots__/post_attachment_opengraph.test.js.snap @@ -0,0 +1,187 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`components/post_view/PostAttachmentOpenGraph Match snapshot for large image openGraphData 1`] = ` +
+
+
+
+ + Mattermost + +

+ + Mattermost Private Cloud Messaging + +

+
+
+
+ description + +
+
+
+
+
+ +
+
+
+
+`; + +exports[`components/post_view/PostAttachmentOpenGraph Match snapshot for no openGraphData 1`] = `""`; + +exports[`components/post_view/PostAttachmentOpenGraph Match snapshot for small image openGraphData 1`] = ` +
+
+
+
+ + Mattermost + +

+ + Mattermost Private Cloud Messaging + +

+
+
+
+ description + +
+
+
+
+
+ +
+
+
+
+`; + +exports[`components/post_view/PostAttachmentOpenGraph Match snapshot for with remove preview 1`] = ` +
+
+
+
+ + Mattermost + + +

+ + Mattermost Private Cloud Messaging + +

+
+
+
+ description + +
+
+
+
+
+ +
+
+
+
+`; diff --git a/components/post_view/post_attachment_opengraph/post_attachment_opengraph.jsx b/components/post_view/post_attachment_opengraph/post_attachment_opengraph.jsx index 8c0030982988..09978dc2572c 100644 --- a/components/post_view/post_attachment_opengraph/post_attachment_opengraph.jsx +++ b/components/post_view/post_attachment_opengraph/post_attachment_opengraph.jsx @@ -4,23 +4,13 @@ import PropTypes from 'prop-types'; import React from 'react'; +import SizeAwareImage from 'components/size_aware_image'; import {postListScrollChange} from 'actions/global_actions.jsx'; import * as CommonUtils from 'utils/commons.jsx'; import {PostTypes} from 'utils/constants.jsx'; import {useSafeUrl} from 'utils/url'; import * as Utils from 'utils/utils.jsx'; import {getImageSrc, isSystemMessage} from 'utils/post_utils.jsx'; -import {getFileDimensionsForDisplay} from 'utils/file_utils'; - -const MAX_DIMENSIONS_LARGE_IMAGE = { - maxHeight: 200, - maxWidth: 400, -}; - -const MAX_DIMENSIONS_SMALL_IMAGE = { - maxHeight: 80, - maxWidth: 95, -}; const DIMENSIONS_NEAREST_POINT_IMAGE = { height: 80, @@ -77,7 +67,6 @@ export default class PostAttachmentOpenGraph extends React.PureComponent { const hasLargeImage = metadata && metadata.images && metadata.images[imageUrl] && imageUrl ? this.hasLargeImage(metadata.images[imageUrl]) : false; this.state = { - imageLoaded: Boolean(metadata), hasLargeImage, removePreview, imageUrl, @@ -88,9 +77,6 @@ export default class PostAttachmentOpenGraph extends React.PureComponent { this.mounted = true; if (!this.props.post.metadata) { this.fetchData(this.props.link); - if (this.state.imageUrl) { - this.loadImage(this.state.imageUrl); - } } } @@ -118,12 +104,6 @@ export default class PostAttachmentOpenGraph extends React.PureComponent { } } - componentDidUpdate(prevProps, prevState) { - if (!this.props.post.metadata && (this.state.imageUrl !== prevState.imageUrl)) { - this.loadImage(this.state.imageUrl); - } - } - componentWillUnmount() { this.mounted = false; } @@ -149,26 +129,19 @@ export default class PostAttachmentOpenGraph extends React.PureComponent { return hasLargeImage; } - onImageLoad = (image) => { + onImageLoad = ({width, height}) => { if (!this.mounted) { return; } - const hasLargeImage = this.hasLargeImage({width: image.target.naturalWidth, height: image.target.naturalHeight}); + const hasLargeImage = this.hasLargeImage({width, height}); this.setState({ hasLargeImage, - imageLoaded: true, }); postListScrollChange(); } - loadImage(src) { - const img = new Image(); - img.onload = this.onImageLoad; - img.src = src; - } - imageToggleAnchorTag(imageUrl) { if (imageUrl && this.state.hasLargeImage) { return ( @@ -183,54 +156,44 @@ export default class PostAttachmentOpenGraph extends React.PureComponent { return null; } - wrapInSmallImageContainer(imageElement) { - return ( -
- {imageElement} -
- ); + loadLargeImage(imageUrl) { + if (imageUrl && this.props.isEmbedVisible && this.state.hasLargeImage) { + const {metadata} = this.props.post; + let imagesDimensions = null; + if (metadata && metadata.images && metadata.images[imageUrl]) { + imagesDimensions = metadata.images[imageUrl]; + } + return ( + + ); + } + return null; } - imageTag(imageUrl, renderingForLargeImage = false) { - let element = null; - const {metadata} = this.props.post; - - if ( - imageUrl && renderingForLargeImage === this.state.hasLargeImage && - (!renderingForLargeImage || (renderingForLargeImage && this.props.isEmbedVisible)) - ) { - if (this.state.imageLoaded) { - const imagesDimensions = metadata && metadata.images && metadata.images[imageUrl]; - - if (renderingForLargeImage) { - const imageDimensions = getFileDimensionsForDisplay(imagesDimensions, MAX_DIMENSIONS_LARGE_IMAGE); - - element = ( - - ); - } else { - const imageDimensions = getFileDimensionsForDisplay(imagesDimensions, MAX_DIMENSIONS_SMALL_IMAGE); - element = this.wrapInSmallImageContainer( - - ); - } - } else if (renderingForLargeImage) { - element = ; - } else { - element = this.wrapInSmallImageContainer( - - ); + loadSmallImage(imageUrl) { + if (imageUrl && !this.state.hasLargeImage) { + const {metadata} = this.props.post; + let imagesDimensions = null; + if (metadata && metadata.images && metadata.images[imageUrl]) { + imagesDimensions = metadata.images[imageUrl]; } + return ( +
+ +
+ ); } - return element; + return null; } truncateText(text) { @@ -309,7 +272,7 @@ export default class PostAttachmentOpenGraph extends React.PureComponent { {' '} {this.imageToggleAnchorTag(imageUrl)}
- {this.imageTag(imageUrl, true)} + {this.loadLargeImage(imageUrl)}
@@ -345,7 +308,7 @@ export default class PostAttachmentOpenGraph extends React.PureComponent { {body} - {this.imageTag(imageUrl, false)} + {this.loadSmallImage(imageUrl)} diff --git a/components/post_view/post_attachment_opengraph/post_attachment_opengraph.test.js b/components/post_view/post_attachment_opengraph/post_attachment_opengraph.test.js new file mode 100644 index 000000000000..d60f7e871f45 --- /dev/null +++ b/components/post_view/post_attachment_opengraph/post_attachment_opengraph.test.js @@ -0,0 +1,123 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {shallow} from 'enzyme'; + +import PostAttachmentOpenGraph from './post_attachment_opengraph.jsx'; + +describe('components/post_view/PostAttachmentOpenGraph', () => { + const post = { + id: 'post_id_1', + root_id: 'root_id', + channel_id: 'channel_id', + create_at: 1, + message: 'https://mattermost.com', + metadata: { + images: { + 'http://mattermost.com/OpenGraphImage.jpg': { + height: 100, + width: 100, + }, + }, + }, + }; + + const baseProps = { + post, + link: 'http://mattermost.com', + previewEnabled: true, + isEmbedVisible: true, + enableLinkPreviews: true, + hasImageProxy: true, + currentUser: { + id: '1234', + }, + openGraphData: { + description: 'description', + images: [{ + height: 100, + secure_url: '', + url: 'http://mattermost.com/OpenGraphImage.jpg', + width: 100, + }], + site_name: 'Mattermost', + title: 'Mattermost Private Cloud Messaging', + }, + toggleEmbedVisibility: jest.fn(), + actions: { + editPost: jest.fn(), + getOpenGraphMetadata: jest.fn(), + }, + }; + + test('Match snapshot for no openGraphData', () => { + const props = { + ...baseProps, + openGraphData: null, + }; + + const wrapper = shallow( + + ); + + expect(wrapper).toMatchSnapshot(); + }); + + test('Match snapshot for small image openGraphData', () => { + const wrapper = shallow( + + ); + + expect(wrapper).toMatchSnapshot(); + }); + + test('Match snapshot for large image openGraphData', () => { + const props = { + ...baseProps, + openGraphData: { + ...baseProps.openGraphData, + images: [{ + height: 180, + secure_url: '', + url: 'http://mattermost.com/OpenGraphImage.jpg', + width: 400, + }], + }, + post: { + ...post, + metadata: { + ...post.metadata, + images: { + 'http://mattermost.com/OpenGraphImage.jpg': { + height: 180, + width: 400, + }, + }, + }, + }, + }; + + const wrapper = shallow( + + ); + + expect(wrapper).toMatchSnapshot(); + }); + + test('Match snapshot for with remove preview', () => { + const props = { + ...baseProps, + post: { + ...post, + user_id: '1234', + }, + }; + + const wrapper = shallow( + + ); + + expect(wrapper).toMatchSnapshot(); + }); +}); diff --git a/components/post_view/post_image/post_image.jsx b/components/post_view/post_image/post_image.jsx index aeaa6d31c909..0b718f1864cf 100644 --- a/components/post_view/post_image/post_image.jsx +++ b/components/post_view/post_image/post_image.jsx @@ -88,7 +88,7 @@ export default class PostImage extends React.PureComponent { src={PostUtils.getImageSrc(this.props.link, this.props.hasImageProxy)} dimensions={this.props.dimensions} showLoader={true} - onHeightReceived={this.handleLoadComplete} + onImageLoaded={this.handleLoadComplete} onImageLoadFail={this.handleLoadError} onClick={this.onImageClick} /> diff --git a/components/post_view/post_message_view/__snapshots__/post_message_view.test.jsx.snap b/components/post_view/post_message_view/__snapshots__/post_message_view.test.jsx.snap index 0cd89c3765fe..e8cfdff341ee 100644 --- a/components/post_view/post_message_view/__snapshots__/post_message_view.test.jsx.snap +++ b/components/post_view/post_message_view/__snapshots__/post_message_view.test.jsx.snap @@ -14,7 +14,7 @@ exports[`components/post_view/PostAttachment should match snapshot 1`] = `
- -
-
-
@@ -86,22 +78,23 @@ exports[`components/SingleImageView should match snapshot 2`] = ` style={Object {}} >
-
-
- -
-
-
@@ -206,27 +199,29 @@ exports[`components/SingleImageView should match snapshot, SVG image 2`] = ` className="image-container" style={ Object { - "width": 300, + "height": 150, + "width": "auto", } } >
-
-
`; + +exports[`components/SingleImageView should set loaded state on callback of onImageLoaded on SizeAwareImage component 1`] = ` + +`; diff --git a/components/single_image_view/single_image_view.jsx b/components/single_image_view/single_image_view.jsx index 7089bb76ff4e..e1798006b918 100644 --- a/components/single_image_view/single_image_view.jsx +++ b/components/single_image_view/single_image_view.jsx @@ -6,18 +6,14 @@ import React from 'react'; import {getFilePreviewUrl, getFileUrl} from 'mattermost-redux/utils/file_utils'; -import {getFileDimensionsForDisplay} from 'utils/file_utils'; +import SizeAwareImage from 'components/size_aware_image'; import {FileTypes} from 'utils/constants.jsx'; import { getFileType, - localizeMessage, } from 'utils/utils'; -import LoadingImagePreview from 'components/loading_image_preview'; import ViewImageModal from 'components/view_image'; -const PREVIEW_IMAGE_MAX_WIDTH = 1024; -const PREVIEW_IMAGE_MAX_HEIGHT = 350; const PREVIEW_IMAGE_MIN_DIMENSION = 50; export default class SingleImageView extends React.PureComponent { @@ -37,69 +33,42 @@ export default class SingleImageView extends React.PureComponent { constructor(props) { super(props); - this.state = { loaded: false, showPreviewModal: false, - viewPortWidth: 0, + dimensions: { + width: props.fileInfo.width, + height: props.fileInfo.height, + }, }; - - this.imageLoaded = null; } componentDidMount() { this.mounted = true; - window.addEventListener('resize', this.handleResize); - this.setViewPortWidth(); - this.loadImage(this.props.fileInfo); } - UNSAFE_componentWillReceiveProps(nextProps) { // eslint-disable-line camelcase - this.loadImage(nextProps.fileInfo); - - if (nextProps.isRhsOpen !== this.props.isRhsOpen) { - this.handleResize(); + static getDerivedStateFromProps(props, state) { + if ((props.fileInfo.width !== state.dimensions.width) || props.fileInfo.height !== state.dimensions.height) { + return { + dimensions: { + width: props.fileInfo.width, + height: props.fileInfo.height, + }, + }; } + return null; } componentWillUnmount() { this.mounted = false; - window.removeEventListener('resize', this.handleResize); - } - - handleResize = () => { - this.setViewPortWidth(); } - setViewPortRef = (node) => { - this.viewPort = node; - } - - setViewPortWidth = () => { - if (this.viewPort && this.viewPort.getBoundingClientRect) { - const rect = this.viewPort.getBoundingClientRect(); - this.setState({viewPortWidth: rect.width}); + imageLoaded = () => { + if (this.mounted) { + this.setState({loaded: true}); } } - loadImage = (fileInfo) => { - const {has_preview_image: hasPreviewImage, id} = fileInfo; - const fileURL = getFileUrl(id); - const previewURL = hasPreviewImage ? getFilePreviewUrl(id) : fileURL; - - const loaderImage = new Image(); - - loaderImage.src = previewURL; - loaderImage.onload = () => { - if (this.imageLoaded) { - this.imageLoaded.src = previewURL; - } - if (this.mounted) { - this.setState({loaded: true}); - } - }; - } - handleImageClick = (e) => { e.preventDefault(); this.setState({showPreviewModal: true}); @@ -109,10 +78,6 @@ export default class SingleImageView extends React.PureComponent { this.setState({showPreviewModal: false}); } - setImageLoadedRef = (node) => { - this.imageLoaded = node; - } - toggleEmbedVisibility = () => { this.props.actions.toggleEmbedVisibility(this.props.postId); } @@ -121,14 +86,14 @@ export default class SingleImageView extends React.PureComponent { const {fileInfo} = this.props; const { loaded, - viewPortWidth, } = this.state; - const maxWidth = viewPortWidth !== 0 && viewPortWidth < PREVIEW_IMAGE_MAX_WIDTH ? viewPortWidth : PREVIEW_IMAGE_MAX_WIDTH; - const dimensions = getFileDimensionsForDisplay(fileInfo, {maxHeight: PREVIEW_IMAGE_MAX_HEIGHT, maxWidth}); + const {has_preview_image: hasPreviewImage, id} = fileInfo; + const fileURL = getFileUrl(id); + const previewURL = hasPreviewImage ? getFilePreviewUrl(id) : fileURL; - const previewHeight = dimensions.height; - const previewWidth = dimensions.width; + const previewHeight = fileInfo.height; + const previewWidth = fileInfo.width; let minPreviewClass = ''; if ( @@ -161,37 +126,25 @@ export default class SingleImageView extends React.PureComponent { ); - const loading = localizeMessage('view_image.loading', 'Loading'); - let viewImageModal; - let loadingImagePreview; let fadeInClass = ''; - let height = previewHeight; - if (height < PREVIEW_IMAGE_MIN_DIMENSION) { - height = PREVIEW_IMAGE_MIN_DIMENSION; - } - - let width = previewWidth; - if (width < PREVIEW_IMAGE_MIN_DIMENSION) { - width = PREVIEW_IMAGE_MIN_DIMENSION; - } - const fileType = getFileType(fileInfo.extension); - let svgClass = ''; - let imageStyle = {height}; - let imageLoadedStyle = {height}; + let styleIfSvgWithDimentions = {}; let imageContainerStyle = {}; - if (width < viewPortWidth && height === PREVIEW_IMAGE_MAX_HEIGHT) { - imageContainerStyle = {width}; - } else if (fileType === FileTypes.SVG) { - svgClass = 'post-image normal'; - imageStyle = {}; - imageLoadedStyle = {}; - imageContainerStyle = { - width: viewPortWidth < PREVIEW_IMAGE_MAX_HEIGHT ? viewPortWidth : PREVIEW_IMAGE_MAX_HEIGHT, - height: PREVIEW_IMAGE_MAX_HEIGHT, - }; + let svgClass = ''; + if (fileType === FileTypes.SVG) { + svgClass = 'svg'; + if (this.state.dimensions.height) { + styleIfSvgWithDimentions = { + width: '100%', + }; + } else { + imageContainerStyle = { + height: 150, + width: 'auto', + }; + } } if (loaded) { @@ -204,39 +157,13 @@ export default class SingleImageView extends React.PureComponent { ); fadeInClass = 'image-fade-in'; - imageStyle = {cursor: 'pointer'}; - imageLoadedStyle = {}; - - if (fileType === FileTypes.SVG) { - imageContainerStyle = {width: viewPortWidth < PREVIEW_IMAGE_MAX_HEIGHT ? viewPortWidth : PREVIEW_IMAGE_MAX_HEIGHT}; - } - } else if (this.props.isEmbedVisible) { - loadingImagePreview = ( - - ); - } - - let image; - if (this.props.isEmbedVisible) { - image = ( - - ); } return (
{fileHeader} @@ -246,14 +173,17 @@ export default class SingleImageView extends React.PureComponent { style={imageContainerStyle} >
- {image} -
-
- {loadingImagePreview} +
} diff --git a/components/single_image_view/single_image_view.test.jsx b/components/single_image_view/single_image_view.test.jsx index c9d0bebfc370..3efdf5669884 100644 --- a/components/single_image_view/single_image_view.test.jsx +++ b/components/single_image_view/single_image_view.test.jsx @@ -5,6 +5,7 @@ import React from 'react'; import {shallow} from 'enzyme'; import SingleImageView from 'components/single_image_view/single_image_view.jsx'; +import SizeAwareImage from 'components/size_aware_image'; describe('components/SingleImageView', () => { const baseProps = { @@ -55,31 +56,6 @@ describe('components/SingleImageView', () => { expect(wrapper).toMatchSnapshot(); }); - test('should call setViewPortWidth on handleResize', () => { - const wrapper = shallow( - - ); - - const instance = wrapper.instance(); - instance.setViewPortWidth = jest.fn(); - - instance.handleResize(); - expect(instance.setViewPortWidth).toHaveBeenCalledTimes(1); - }); - - test('should match state on setViewPortWidth', () => { - const wrapper = shallow( - - ); - - wrapper.setState({viewPortWidth: 300}); - const instance = wrapper.instance(); - instance.viewPort = {getBoundingClientRect: () => ({width: 500})}; - instance.setViewPortWidth(); - - expect(wrapper.state('viewPortWidth')).toEqual(500); - }); - test('should match state on handleImageClick', () => { const wrapper = shallow( @@ -117,4 +93,14 @@ describe('components/SingleImageView', () => { expect(props.actions.toggleEmbedVisibility).toHaveBeenCalledTimes(1); expect(props.actions.toggleEmbedVisibility).toBeCalledWith('original_post_id'); }); + + test('should set loaded state on callback of onImageLoaded on SizeAwareImage component', () => { + const wrapper = shallow( + + ); + expect(wrapper.state('loaded')).toEqual(false); + wrapper.find(SizeAwareImage).prop('onImageLoaded')(); + expect(wrapper.state('loaded')).toEqual(true); + expect(wrapper).toMatchSnapshot(); + }); }); diff --git a/components/size_aware_image.jsx b/components/size_aware_image.jsx index f82282404a1f..c18970f7d335 100644 --- a/components/size_aware_image.jsx +++ b/components/size_aware_image.jsx @@ -5,7 +5,7 @@ import PropTypes from 'prop-types'; import React from 'react'; import LoadingImagePreview from 'components/loading_image_preview'; -import {createPlaceholderImage, loadImage} from 'utils/image_utils'; +import {loadImage} from 'utils/image_utils'; const WAIT_FOR_HEIGHT_TIMEOUT = 100; @@ -32,12 +32,17 @@ export default class SizeAwareImage extends React.PureComponent { /* * A callback that is called as soon as the image component has a height value */ - onHeightReceived: PropTypes.func.isRequired, + onImageLoaded: PropTypes.func, /* * A callback that is called when image load fails */ onImageLoadFail: PropTypes.func, + + /* + * css classes that can added to the img as well as parent div on svg for placeholder + */ + className: PropTypes.string, } constructor(props) { @@ -51,6 +56,7 @@ export default class SizeAwareImage extends React.PureComponent { } componentDidMount() { + this.mounted = true; this.loadImage(); } @@ -61,13 +67,14 @@ export default class SizeAwareImage extends React.PureComponent { } componentWillUnmount() { + this.mounted = false; this.stopWaitingForHeight(); } loadImage = () => { const image = loadImage(this.props.src, this.handleLoad); - image.onerror = this.handleError(); + image.onerror = this.handleError; if (!this.props.dimensions) { this.waitForHeight(image); @@ -76,8 +83,8 @@ export default class SizeAwareImage extends React.PureComponent { waitForHeight = (image) => { if (image && image.height) { - if (this.props.onHeightReceived) { - this.props.onHeightReceived(image.height); + if (this.props.onImageLoaded) { + this.props.onImageLoaded({height: image.height, width: image.width}); } this.heightTimeout = 0; } else { @@ -95,43 +102,40 @@ export default class SizeAwareImage extends React.PureComponent { } handleLoad = (image) => { - if (this.props.onHeightReceived && image.height) { - this.props.onHeightReceived(image.height); + if (this.mounted) { + if (this.props.onImageLoaded && image.height) { + this.props.onImageLoaded({height: image.height, width: image.width}); + } + this.setState({ + loaded: true, + error: false, + }); } - - this.setState({ - loaded: true, - error: false, - }); }; handleError = () => { - this.stopWaitingForHeight(); - if (this.props.onImageLoadFail) { - this.props.onImageLoadFail(); + if (this.mounted) { + this.stopWaitingForHeight(); + if (this.props.onImageLoadFail) { + this.props.onImageLoadFail(); + } + this.setState({error: true}); } - this.setState({error: true}); }; render() { const { dimensions, + src, ...props } = this.props; Reflect.deleteProperty(props, 'showLoader'); - Reflect.deleteProperty(props, 'onHeightReceived'); + Reflect.deleteProperty(props, 'onImageLoaded'); Reflect.deleteProperty(props, 'onImageLoadFail'); - let src; - if (!this.state.loaded && dimensions) { - // Generate a blank image as a placeholder because that will scale down to fit the available space - // while maintaining the correct aspect ratio - src = createPlaceholderImage(dimensions.width, dimensions.height); - } else if (this.state.error) { + if (this.state.error) { return null; - } else { - src = this.props.src; } return ( @@ -143,10 +147,22 @@ export default class SizeAwareImage extends React.PureComponent { />
} - + {dimensions && dimensions.width && !this.state.loaded ? ( +
+
+ +
+
+ ) : ( + + )} ); } diff --git a/components/size_aware_image.test.jsx b/components/size_aware_image.test.jsx index 2dc309716a9c..4f304b21d46b 100644 --- a/components/size_aware_image.test.jsx +++ b/components/size_aware_image.test.jsx @@ -9,7 +9,7 @@ import LoadingImagePreview from 'components/loading_image_preview'; jest.mock('utils/image_utils'); -import {createPlaceholderImage, loadImage} from 'utils/image_utils'; +import {loadImage} from 'utils/image_utils'; describe('components/SizeAwareImage', () => { const baseProps = { @@ -17,24 +17,20 @@ describe('components/SizeAwareImage', () => { height: 200, width: 300, }, - onHeightReceived: jest.fn(), + onImageLoaded: jest.fn(), src: 'https://example.com/image.png', }; loadImage.mockReturnValue(() => ({})); - test('should render a placeholder when first mounted with dimensions', () => { - createPlaceholderImage.mockImplementation(() => 'data:image/png;base64,abc123'); - + test('should render an svg when first mounted with dimensions', () => { const wrapper = mount(); - const src = wrapper.find('img').prop('src'); - expect(src.startsWith('data:image')).toBeTruthy(); + const viewBox = wrapper.find('svg').prop('viewBox'); + expect(viewBox).toEqual('0 0 300 200'); }); test('should render a placeholder and has loader when showLoader is true', () => { - const placeholder = 'data:image/png;base64,abc123'; - createPlaceholderImage.mockImplementation(() => placeholder); const props = { ...baseProps, showLoader: true, @@ -42,7 +38,6 @@ describe('components/SizeAwareImage', () => { const wrapper = shallow(); expect(wrapper.find(LoadingImagePreview).exists()).toEqual(true); - expect(wrapper.find('img').prop('src')).toEqual(placeholder); expect(wrapper).toMatchSnapshot(); }); @@ -79,10 +74,11 @@ describe('components/SizeAwareImage', () => { expect(loadImage.mock.calls[1][0]).toEqual(newSrc); }); - test('should call onHeightReceived on image is loaded', () => { + test('should call onImageLoaded on image is loaded', () => { const height = 123; + const width = 1234; loadImage.mockImplementation((src, onLoad) => { - onLoad({height}); + onLoad({height, width}); return {}; }); @@ -90,7 +86,7 @@ describe('components/SizeAwareImage', () => { const props = {...baseProps}; shallow(); - expect(baseProps.onHeightReceived).toHaveBeenCalledWith(height); + expect(baseProps.onImageLoaded).toHaveBeenCalledWith({height, width}); }); test('should call onImageLoadFail when image load fails and should render empty/null', () => { diff --git a/plugins/test/__snapshots__/post_type.test.jsx.snap b/plugins/test/__snapshots__/post_type.test.jsx.snap index dc39775824bc..b929b853daeb 100644 --- a/plugins/test/__snapshots__/post_type.test.jsx.snap +++ b/plugins/test/__snapshots__/post_type.test.jsx.snap @@ -78,7 +78,7 @@ exports[`plugins/PostMessageView should match snapshot with no extended post typ widthRatio) { - return { - height: maxHeight, - width: width * (1 / heightRatio), - }; - } - - return { - height: height * (1 / widthRatio), - width: maxWidth, - }; -} - export function getFileTypeFromMime(mimetype) { const mimeTypeSplitBySlash = mimetype.split('/'); const mimeTypePrefix = mimeTypeSplitBySlash[0]; diff --git a/utils/file_utils.test.jsx b/utils/file_utils.test.jsx index d4d4e1a255a7..fd12eea3ec93 100644 --- a/utils/file_utils.test.jsx +++ b/utils/file_utils.test.jsx @@ -6,7 +6,6 @@ import assert from 'assert'; import { trimFilename, canUploadFiles, - getFileDimensionsForDisplay, getFileTypeFromMime, } from 'utils/file_utils.jsx'; import * as UserAgent from 'utils/user_agent'; @@ -110,35 +109,3 @@ describe('FileUtils.canUploadFiles', () => { }); }); }); - -describe('FileUtils.getFileDimensionsForDisplay', () => { - it('return image dimensions as they are smaller than max dimensions', () => { - const expectedDimensions = getFileDimensionsForDisplay({height: 200, width: 200}, {maxHeight: 300, maxWidth: 300}); - expect(expectedDimensions).toEqual({height: 200, width: 200}); - }); - - it('return image dimensions based on height dimetions as ratio of height > width', () => { - const expectedDimensions = getFileDimensionsForDisplay({height: 600, width: 400}, {maxHeight: 300, maxWidth: 300}); - expect(expectedDimensions).toEqual({height: 300, width: 200}); - }); - - it('return image dimensions based on width dimetions as ratio of width > height', () => { - const expectedDimensions = getFileDimensionsForDisplay({height: 400, width: 600}, {maxHeight: 300, maxWidth: 300}); - expect(expectedDimensions).toEqual({height: 200, width: 300}); - }); - - it('return image dimensions based on width ratio', () => { - const expectedDimensions = getFileDimensionsForDisplay({height: 200, width: 600}, {maxHeight: 300, maxWidth: 300}); - expect(expectedDimensions).toEqual({height: 100, width: 300}); - }); - - it('return image dimensions based on height ratio', () => { - const expectedDimensions = getFileDimensionsForDisplay({height: 600, width: 200}, {maxHeight: 300, maxWidth: 300}); - expect(expectedDimensions).toEqual({height: 300, width: 100}); - }); - - it('return null if dimensions does not exists', () => { - const expectedDimensions = getFileDimensionsForDisplay(null, {maxHeight: 300, maxWidth: 300}); - expect(expectedDimensions).toEqual(null); - }); -}); diff --git a/utils/image_utils.jsx b/utils/image_utils.jsx index f7d298170fb2..d4f3c5ef4b3b 100644 --- a/utils/image_utils.jsx +++ b/utils/image_utils.jsx @@ -1,22 +1,6 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -// createPlaceholderImage returns a data URI for a blank image with the given dimensions. -export function createPlaceholderImage(width, height) { - const placeholder = document.createElement('canvas'); - - placeholder.width = width; - placeholder.height = height; - placeholder.style.backgroundColor = 'transparent'; - - try { - return placeholder.toDataURL(); - } catch (e) { - // Canvas.toDataURL throws an error if the dimensions are too large - return ''; - } -} - // loadImage loads an image in the background without rendering it or using an XMLHttpRequest. export function loadImage(src, onLoad) { const image = new Image(); From 9ce3e061d5743438da51e8d67fb1a021f647dcbe Mon Sep 17 00:00:00 2001 From: sudheer Date: Wed, 20 Mar 2019 16:13:06 +0530 Subject: [PATCH 2/2] Fix review comments --- .../size_aware_image.test.jsx.snap | 25 ++++---- .../post_body_additional_content.jsx | 14 ----- .../post_view/post_image/post_image.jsx | 16 ----- components/size_aware_image.jsx | 62 +++++++++++-------- components/size_aware_image.test.jsx | 7 ++- 5 files changed, 53 insertions(+), 71 deletions(-) diff --git a/components/__snapshots__/size_aware_image.test.jsx.snap b/components/__snapshots__/size_aware_image.test.jsx.snap index 7405f6712b22..1348b8eafdf6 100644 --- a/components/__snapshots__/size_aware_image.test.jsx.snap +++ b/components/__snapshots__/size_aware_image.test.jsx.snap @@ -5,7 +5,10 @@ exports[`components/SizeAwareImage should render a placeholder and has loader wh
@@ -16,19 +19,17 @@ exports[`components/SizeAwareImage should render a placeholder and has loader wh
-
- -
+ } + viewBox="0 0 300 200" + xmlns="http://www.w3.org/2000/svg" + />
`; diff --git a/components/post_view/post_body_additional_content/post_body_additional_content.jsx b/components/post_view/post_body_additional_content/post_body_additional_content.jsx index ca86e10b0f37..f5b1b30326e5 100644 --- a/components/post_view/post_body_additional_content/post_body_additional_content.jsx +++ b/components/post_view/post_body_additional_content/post_body_additional_content.jsx @@ -69,7 +69,6 @@ export default class PostBodyAdditionalContent extends React.PureComponent { this.generateToggleableEmbed = this.generateToggleableEmbed.bind(this); this.generateStaticEmbed = this.generateStaticEmbed.bind(this); this.isLinkToggleable = this.isLinkToggleable.bind(this); - this.handleLinkLoadError = this.handleLinkLoadError.bind(this); this.handleLinkLoaded = this.handleLinkLoaded.bind(this); this.state = { @@ -150,10 +149,6 @@ export default class PostBodyAdditionalContent extends React.PureComponent { image.onload = () => { this.handleLinkLoaded(); }; - - image.onerror = () => { - this.handleLinkLoadError(); - }; } } @@ -191,14 +186,6 @@ export default class PostBodyAdditionalContent extends React.PureComponent { return false; } - handleLinkLoadError() { - if (this.mounted) { - this.setState({ - linkLoadError: true, - }); - } - } - handleLinkLoaded() { if (this.mounted) { this.setState({ @@ -236,7 +223,6 @@ export default class PostBodyAdditionalContent extends React.PureComponent { { - if (!this.mounted) { - return; - } - - if (this.props.onLinkLoadError) { - this.props.onLinkLoadError(); - } - } - onImageClick = (e) => { e.preventDefault(); this.props.handleImageClick(); @@ -89,7 +74,6 @@ export default class PostImage extends React.PureComponent { dimensions={this.props.dimensions} showLoader={true} onImageLoaded={this.handleLoadComplete} - onImageLoadFail={this.handleLoadError} onClick={this.onImageClick} />
diff --git a/components/size_aware_image.jsx b/components/size_aware_image.jsx index c18970f7d335..8b859c1aeefc 100644 --- a/components/size_aware_image.jsx +++ b/components/size_aware_image.jsx @@ -123,46 +123,54 @@ export default class SizeAwareImage extends React.PureComponent { } }; - render() { + renderImageLoaderIfNeeded = () => { + if (!this.state.loaded && this.props.showLoader && !this.state.error) { + return ( +
+ +
+ ); + } + return null; + } + + renderImageOrPlaceholder = () => { const { dimensions, src, ...props } = this.props; + if (dimensions && dimensions.width && !this.state.loaded) { + return ( +
+ +
+ ); + } Reflect.deleteProperty(props, 'showLoader'); Reflect.deleteProperty(props, 'onImageLoaded'); Reflect.deleteProperty(props, 'onImageLoadFail'); - if (this.state.error) { - return null; - } + return ( + + ); + } + render() { return ( - {!this.state.loaded && this.props.showLoader && -
- -
- } - {dimensions && dimensions.width && !this.state.loaded ? ( -
-
- -
-
- ) : ( - - )} + {this.renderImageLoaderIfNeeded()} + {this.renderImageOrPlaceholder()} ); } diff --git a/components/size_aware_image.test.jsx b/components/size_aware_image.test.jsx index 4f304b21d46b..5e38ef4c5eef 100644 --- a/components/size_aware_image.test.jsx +++ b/components/size_aware_image.test.jsx @@ -89,15 +89,18 @@ describe('components/SizeAwareImage', () => { expect(baseProps.onImageLoaded).toHaveBeenCalledWith({height, width}); }); - test('should call onImageLoadFail when image load fails and should render empty/null', () => { + test('should call onImageLoadFail when image load fails and should have svg', () => { const onImageLoadFail = jest.fn(); const props = {...baseProps, onImageLoadFail}; - const wrapper = shallow(); + const wrapper = mount(); + wrapper.setState({loaded: false}); wrapper.instance().handleError(); expect(props.onImageLoadFail).toHaveBeenCalled(); + expect(wrapper.state('error')).toBe(true); expect(wrapper.find('img').exists()).toEqual(false); + expect(wrapper.find('svg').exists()).toEqual(true); expect(wrapper.find(LoadingImagePreview).exists()).toEqual(false); }); });