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 28 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
138 changes: 92 additions & 46 deletions packages/ui/ImageItem/ImageItem.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
/* 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.
*
Expand All @@ -14,13 +18,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 @@ -36,14 +45,6 @@ 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 +95,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 +104,8 @@ const ImageItemBase = kind({
selected: false
},

functional: true,

styles: {
css: componentCss,
className: 'imageItem',
Expand All @@ -123,42 +117,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.ImageItemBase.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.ImageItemBase.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
61 changes: 61 additions & 0 deletions packages/ui/ImageItem/MemoPropsDecorator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
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();

const defaultConfig = {
filter: []
};

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;
});

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