Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLAT-109381: Reduce the ImageItem render time #2790

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 97 additions & 46 deletions packages/ui/ImageItem/ImageItem.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
/* eslint-disable react-hooks/rules-of-hooks */
/* eslint-disable react-hooks/exhaustive-deps */
// To use `React.useMemo` in a kind, the above eslint rules have been disabled.

/**
* Unstyled image item components and behaviors to be customized by a theme or application.
*
* @module ui/ImageItem
* @exports ImageItem
* @exports ImageItemBase
* @exports MemoPropsDecorator
* @exports MemoPropsThemeContextConsumer
* @exports MemoPropsThemeDecorator
*/

import EnactPropTypes from '@enact/core/internal/prop-types';
Expand All @@ -14,13 +22,18 @@ import ComponentOverride from '../ComponentOverride';
import Image from '../Image';
import {Cell, Column, Row} from '../Layout';

import {MemoPropsContextConsumer, MemoPropsDecorator, MemoPropsThemeContextConsumer, MemoPropsThemeDecorator} from './MemoPropsDecorator';

import componentCss from './ImageItem.module.less';

// Adapts ComponentOverride to work within Cell since both use the component prop
function ImageOverride ({imageComponent, ...rest}) {
return ComponentOverride({
component: imageComponent,
...rest
return MemoPropsContextConsumer((context) => {
return ComponentOverride({
...rest,
component: imageComponent,
src: context && context.src
});
});
}

Expand All @@ -29,21 +42,14 @@ function ImageOverride ({imageComponent, ...rest}) {
*
* @class ImageItem
* @memberof ui/ImageItem
* @mixes ui/ImageItem.MemoPropsThemeDecorator
* @ui
* @public
*/
const ImageItemBase = kind({
name: 'ui:ImageItem',

propTypes: /** @lends ui/ImageItem.ImageItem.prototype */ {
/**
* The caption displayed with the image.
*
* @type {Node}
* @public
*/
children: PropTypes.node,

/**
* Customizes the component by mapping the supplied collection of CSS class names to the
* corresponding internal elements and states of this component.
Expand Down Expand Up @@ -94,16 +100,7 @@ const ImageItemBase = kind({
* @default false
* @public
*/
selected: PropTypes.bool,

/**
* String value or Object of values used to determine which image will appear on a specific
* screenSize.
*
* @type {String|Object}
* @public
*/
src: PropTypes.oneOfType([PropTypes.string, PropTypes.object])
selected: PropTypes.bool
},

defaultProps: {
Expand All @@ -112,6 +109,8 @@ const ImageItemBase = kind({
selected: false
},

functional: true,

styles: {
css: componentCss,
className: 'imageItem',
Expand All @@ -123,42 +122,94 @@ const ImageItemBase = kind({
selected,
horizontal: orientation === 'horizontal',
vertical: orientation === 'vertical'
})
}),
memoizedImageCell: ({css, imageComponent, orientation, placeholder}) => {
const isHorizontal = orientation === 'horizontal';

return React.useMemo(() => {
return (
<Cell
className={css.image}
component={ImageOverride}
imageComponent={imageComponent}
key="image"
placeholder={placeholder}
shrink={isHorizontal}
/>
);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [css.image, imageComponent, isHorizontal, placeholder]);
},
memoizedChildrenCell: ({css, orientation}) => {
const isHorizontal = orientation === 'horizontal';

return React.useMemo(() => {
return (
<Cell
// eslint-disable-next-line no-undefined
align={isHorizontal ? 'center' : undefined}
className={css.caption}
key="children"
shrink={!isHorizontal}
>
{MemoPropsContextConsumer((context) => {
return context && context.children;
})}
</Cell>
);
}, [css.caption, isHorizontal]);
}
},

render: ({children, css, imageComponent, orientation, placeholder, src, ...rest}) => {
render: ({className, memoizedChildrenCell, memoizedImageCell, orientation, ...rest}) => {
delete rest.css;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to delete css, I believe it's non-enumerable.

delete rest.imageComponent;
delete rest.placeholder;
delete rest.selected;

const isHorizontal = orientation === 'horizontal';
const Component = isHorizontal ? Row : Column;

return (
<Component {...rest}>
<Cell
className={css.image}
component={ImageOverride}
imageComponent={imageComponent}
placeholder={placeholder}
shrink={isHorizontal}
src={src}
/>
{children ? (
<Cell
className={css.caption}
shrink={!isHorizontal}
// eslint-disable-next-line no-undefined
align={isHorizontal ? 'center' : undefined}
>
{children}
</Cell>
) : null}
</Component>
<div {...rest} className={className}>
{React.useMemo(() => {
return (
<Component>
{[memoizedImageCell, memoizedChildrenCell]}
</Component>
);
}, [Component])}
</div>
);
}
});

export default ImageItemBase;
const ImageItem = MemoPropsDecorator({filter: ['children', 'src']})(ImageItemBase);

/**
* The caption displayed with the image.
*
* @name children
* @memberof ui/ImageItem.ImageItem.prototype
* @type {Node}
* @public
*/

/**
* String value or Object of values used to determine which image will appear on a specific
* screenSize.
*
* @name src
* @memberof ui/ImageItem.ImageItem.prototype
* @type {String|Object}
* @public
*/

export default ImageItem;
export {
ImageItemBase as ImageItem,
ImageItemBase
ImageItem,
ImageItemBase,
MemoPropsDecorator,
MemoPropsThemeContextConsumer,
MemoPropsThemeDecorator
};
7 changes: 4 additions & 3 deletions packages/ui/ImageItem/ImageItem.module.less
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,25 @@
//
.imageItem {
box-sizing: border-box;
height: 100%;
MikyungKim marked this conversation as resolved.
Show resolved Hide resolved

.image {
display: block; // leaving this to prevent whitespace between inline-block elements
margin: 0;
}

.caption,
&.selected {
/* Public Class Names */
}

&.horizontal {
.image {
width: 25%;
height: 100%;
}
}

&.vertical {
.image {
width: 100%;
Expand Down
97 changes: 97 additions & 0 deletions packages/ui/ImageItem/MemoPropsDecorator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import hoc from '@enact/core/hoc';
import pick from 'ramda/src/pick';
import omit from 'ramda/src/omit';
import React from 'react';

const MemoPropsThemeContext = React.createContext();
const MemoPropsContext = React.createContext();

/**
* Default config for `MemoPropsDecorator`.
*
* @memberof ui/ImageItem.MemoPropsDecorator
* @hocconfig
*/
const defaultConfig = {
/**
* Array of the props passed throught a context.
*
* @type {String[]}
* @default []
* @private
* @memberof ui/ImageItem.MemoPropsDecorator.defaultConfig
*/
filter: []
};

/**
* A higher-order component that is used to provide a context provider.
* The props which keys are defined in the array of the `filter` config,
* will be passed through a context. The other props will be passed
* as props.
*
* @class MemoPropsDecorator
* @memberof ui/ImageItem
* @hoc
* @public
*/
const MemoPropsDecorator = hoc(defaultConfig, (config, Wrapped) => {
const {filter} = config;

// eslint-disable-next-line no-shadow
function MemoPropsDecorator (props) {
const picked = pick(filter, props) || {};
const omitted = omit(filter, props) || {};

return (
<MemoPropsContext.Provider value={picked}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This will force the component to update every time it receives props, even if the values are exactly the same, because the object has changed and context will force the update. Some form of memoization should be done if this truly is the desired solution.

<Wrapped {...omitted} />
</MemoPropsContext.Provider>
);
}

return MemoPropsDecorator;
});

/**
* A higher-order component that is used to provide a context provider.
* All the props will be passed through a context and will be passed as props.
*
* There are no configurable options on this HOC.
*
* @class MemoPropsThemeDecorator
* @memberof ui/ImageItem
* @hoc
* @public
*/
const MemoPropsThemeDecorator = hoc((config, Wrapped) => {
// eslint-disable-next-line no-shadow
function MemoPropsThemeDecorator (props) {
return (
<MemoPropsThemeContext.Provider value={props}>
<Wrapped {...props} />
</MemoPropsThemeContext.Provider>
);
}

return MemoPropsThemeDecorator;
});

const ContextConsumer = (Context) => (fn) => { // eslint-disable-line enact/display-name
return (
<Context.Consumer>
{fn}
</Context.Consumer>
);
};

const MemoPropsContextConsumer = ContextConsumer(MemoPropsContext);
const MemoPropsThemeContextConsumer = ContextConsumer(MemoPropsThemeContext);

export default MemoPropsThemeContext;
export {
MemoPropsContextConsumer,
MemoPropsDecorator,
MemoPropsThemeContextConsumer,
MemoPropsThemeDecorator
};
Loading