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.

Sign up for GitHub

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

Improve Expensicon lib #1188

Merged
merged 23 commits into from
Jan 26, 2021
Merged

Improve Expensicon lib #1188

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
649898c
Improve scalability of Expensicon lib
roryabraham Jan 7, 2021
4c88a0c
rename iconSizeSmall to iconSizeNormal
roryabraham Jan 7, 2021
07732e1
remove unused style
roryabraham Jan 7, 2021
ee93a19
Separate brand assets from expensicons, export components not strings
roryabraham Jan 7, 2021
6fa3400
Clean up exports
roryabraham Jan 7, 2021
7fdc2d1
Separate imports from Icon module
roryabraham Jan 7, 2021
17f1bfb
Use named exports instead of defaults
roryabraham Jan 7, 2021
2b2f70f
Use PascalCase instead of UPPER_SNAKE
roryabraham Jan 7, 2021
0b2c146
add props docs
roryabraham Jan 7, 2021
125d18f
include more svg assets
roryabraham Jan 9, 2021
81ed7b0
Use svg icons throughout app
roryabraham Jan 10, 2021
8f05188
Remove unused png assets
roryabraham Jan 10, 2021
3a1ca8b
Merge master into Rory-ImproveExpensicons
roryabraham Jan 20, 2021
bf6b516
Fix merge issues, make icon animatable
roryabraham Jan 20, 2021
09f1452
Get rid of isEnabled prop
roryabraham Jan 21, 2021
d76e860
Simplify fill color logic
roryabraham Jan 21, 2021
e129576
fix fill color logic
roryabraham Jan 21, 2021
7415d5f
Fix prop comments
roryabraham Jan 24, 2021
4a0e4b1
rename icon prop to src
roryabraham Jan 24, 2021
df026f9
Fix AnimatedIcon icon prop
roryabraham Jan 24, 2021
6ee53f0
Merge branch 'master' into Rory-ImproveExpensicons
roryabraham Jan 24, 2021
a6f4ca8
Make more icon size variables
roryabraham Jan 24, 2021
e6a7727
Remove brand assets file and simplify fill logic
roryabraham Jan 25, 2021
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
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
"html-entities": "^1.3.1",
"lodash.get": "^4.4.2",
"lodash.has": "^4.5.2",
"lodash.merge": "^4.6.2",
"lodash.orderby": "^4.6.0",
"metro-config": "^0.64.0",
"modal-enhanced-react-native-web": "^0.2.0",
Expand Down
26 changes: 0 additions & 26 deletions src/components/Expensicons/ExpensifyCashLogoIcon.js
View file
Open in desktop

This file was deleted.

28 changes: 0 additions & 28 deletions src/components/Expensicons/PinIcon.js
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions src/components/Expensicons/index.js
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions src/components/Icon/BRAND_ASSETS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import ExpensifyCashLogoSVG from '../../../assets/images/expensify-cash.svg';

export default {
EXPENSIFY_CASH_LOGO: ExpensifyCashLogoSVG,
};
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions src/components/Icon/EXPENSICONS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import PinSVG from '../../../assets/images/pin.svg';

export default {
PIN: PinSVG,
};
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
49 changes: 49 additions & 0 deletions src/components/Icon/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import _ from 'underscore';
import React, {memo} from 'react';
import PropTypes from 'prop-types';
import themeColors from '../../styles/themes/default';
import variables from '../../styles/variables';
import BRAND_ASSETS from './BRAND_ASSETS';
import EXPENSICONS from './EXPENSICONS';

const ICONS = _.extend(BRAND_ASSETS, EXPENSICONS);

const propTypes = {
icon: PropTypes.oneOf(_.values(ICONS)).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate this but I think this could just be validated that it's some kind of component rather than need to validate that it's one of the large list of icon components and it would simplify this a bit.

roryabraham marked this conversation as resolved.
Show resolved Hide resolved
width: PropTypes.number,
height: PropTypes.number,
isEnabled: PropTypes.bool,
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
};

const defaultProps = {
width: variables.iconSizeNormal,
height: variables.iconSizeNormal,
isEnabled: false,
};

const Icon = (props) => {
const IconToRender = props.icon;
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
let fillColor = props.isEnabled ? themeColors.heading : themeColors.icon;

// Do not pass a fill color for brand assets
if (_.contains(_.values(BRAND_ASSETS), props.icon)) {
fillColor = undefined;
}
roryabraham marked this conversation as resolved.
Show resolved Hide resolved

return (
<IconToRender
width={props.width}
height={props.height}
fill={fillColor}
/>
);
};

Icon.propTypes = propTypes;
Icon.defaultProps = defaultProps;

export default memo(Icon);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we might want to prevent this component from re-rendering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose for memoizing the component was to boost performance. Icons will render often (and almost always with the same props), so this seems like the classic use-case for React.memo.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable to me. I just have a few questions...

Icons will render often

I'm wondering if this will always be the case? Maybe some icons will not render often? And even if they did... is it expensive to render an SVG? Said another way, is there some specific performance issue that we are trying to prevent? Should we just wrap all the things in React.memo() in that case? Is there any reason why we might want to use React.memo() judiciously? Or is it no big deal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an article that provides some pretty good insight on when it is and is not a good idea to use React.memo. This diagram summarizes the heuristic pretty well:

image

As far as I understand, it's a balancing act between the time it takes to compare the props for equality vs the time it takes for the component to render. I think if you had a component with no props (unlikely, but possible), then you would always want to use React.memo, because the equality comparison is close to instantaneous.

The Icon component for sure meets the first three criteria in the diagram above, and it's a bit of a toss-up whether or not it meets the third. I have no idea how to measure and know for sure. Probably not a significant difference either way, but maybe @tgolen would have a good 6th sense as to whether or not this is a good time to use React.memo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for sharing this. I read the article and the impression I have goes something like...

If the time it takes to perform a React.memo() shallow comparison is greater than the time it takes to render a component then we might actually be slowing things down by using React.memo()?

The author does mention to use profiling tools to measure the benefits which sounds like a smart idea, but I might not pull them out in the first place unless something seemed slow.

So, I guess I'm sort of in the camp that we should use memo, PureComponent, etc when it is very clear why they are being used, there is some measurable benefit, and if we do not use them performance will degrade in an obvious way.

That said, if there is no measurable benefit in the other direction and the component still does what it's supposed to maybe it doesn't matter? At least one person in this SO post said you should "always" use it so I really don't know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also whatever we decide we should turn this into a SO post bc this is a good chat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, agreed. I want to wait to see what Tim G has to say about it.

export {
BRAND_ASSETS,
EXPENSICONS,
};
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 3 additions & 2 deletions src/pages/SetPasswordPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import lodashHas from 'lodash.has';
import compose from '../libs/compose';
import {Redirect, withRouter} from '../libs/Router';
import styles from '../styles/styles';
import {ExpensifyCashLogoIcon} from '../components/Expensicons';
import Icon, {BRAND_ASSETS} from '../components/Icon';
import CustomStatusBar from '../components/CustomStatusBar';
import {setPassword} from '../libs/actions/Session';
import ONYXKEYS from '../ONYXKEYS';
Expand Down Expand Up @@ -94,7 +94,8 @@ class SetPasswordPage extends Component {
<SafeAreaView style={[styles.signInPage]}>
<View style={[styles.signInPageInner]}>
<View style={[styles.signInPageLogo]}>
<ExpensifyCashLogoIcon
<Icon
icon={BRAND_ASSETS.EXPENSIFY_CASH_LOGO}
width={variables.componentSizeLarge}
height={variables.componentSizeLarge}
/>
Expand Down
4 changes: 2 additions & 2 deletions src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import styles from '../../styles/styles';
import ONYXKEYS from '../../ONYXKEYS';
import {withRouter} from '../../libs/Router';
import LHNToggle from '../../../assets/images/icon-menu-toggle.png';
import {PinIcon} from '../../components/Expensicons';
import Icon, {EXPENSICONS} from '../../components/Icon';
import compose from '../../libs/compose';
import {togglePinnedState} from '../../libs/actions/Report';

Expand Down Expand Up @@ -70,7 +70,7 @@ const HeaderView = props => (
onPress={() => togglePinnedState(props.report)}
style={[styles.touchableButtonImage, styles.mr0]}
>
<PinIcon height={20} width={20} isEnabled={props.report.isPinned} />
<Icon icon={EXPENSICONS.PIN} isEnabled={props.report.isPinned} />
</TouchableOpacity>
</View>
</View>
Expand Down
9 changes: 7 additions & 2 deletions src/pages/home/sidebar/ChatSwitcherSearchForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import {
import PropTypes from 'prop-types';
import styles from '../../../styles/styles';
import themeColors from '../../../styles/themes/default';
import {ExpensifyCashLogoIcon} from '../../../components/Expensicons';
import variables from '../../../styles/variables';
import Icon, {BRAND_ASSETS} from '../../../components/Icon';
import TextInputWithFocusStyles from '../../../components/TextInputWithFocusStyles';
import iconX from '../../../../assets/images/icon-x.png';
import {getDisplayName} from '../../../libs/actions/PersonalDetails';
Expand Down Expand Up @@ -59,7 +60,11 @@ const ChatSwitcherSearchForm = props => (
<View style={[styles.flexRow, styles.sidebarHeaderTop]}>
{props.isLogoVisible && (
<View style={[styles.mr3]}>
<ExpensifyCashLogoIcon />
<Icon
icon={BRAND_ASSETS.EXPENSIFY_CASH_LOGO}
width={variables.componentSizeNormal}
height={variables.componentSizeNormal}
/>
</View>
)}

Expand Down
7 changes: 4 additions & 3 deletions src/pages/signin/SignInPageLayout/SignInPageLayoutNarrow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
import PropTypes from 'prop-types';
import styles from '../../../styles/styles';
import variables from '../../../styles/variables';
import {ExpensifyCashLogoIcon} from '../../../components/Expensicons';
import Icon, {BRAND_ASSETS} from '../../../components/Icon';

const propTypes = {
// The children to show inside the layout
Expand All @@ -17,9 +17,10 @@ const SignInPageLayoutNarrow = ({children}) => (
<View>
<View style={[styles.signInPageInnerNative]}>
<View style={[styles.signInPageLogoNative]}>
<ExpensifyCashLogoIcon
<Icon
icon={BRAND_ASSETS.EXPENSIFY_CASH_LOGO}
width={variables.componentSizeLarge}
height={variables.componentSizeNormal}
height={variables.componentSizeLarge}
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
/>
</View>

Expand Down
5 changes: 3 additions & 2 deletions src/pages/signin/SignInPageLayout/SignInPageLayoutWide.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../../styles/styles';
import {ExpensifyCashLogoIcon} from '../../../components/Expensicons';
import Icon, {BRAND_ASSETS} from '../../../components/Icon';
import welcomeScreenshot from '../../../../assets/images/welcome-screenshot-wide.png';
import variables from '../../../styles/variables';

Expand All @@ -27,7 +27,8 @@ const SignInPageLayoutWide = ({children}) => (
</View>
<View style={[styles.flex1, styles.w50]}>
<View style={[styles.signInPageLogo, styles.mt6, styles.mb5]}>
<ExpensifyCashLogoIcon
<Icon
icon={BRAND_ASSETS.EXPENSIFY_CASH_LOGO}
width={variables.componentSizeLarge}
height={variables.componentSizeLarge}
/>
Expand Down
5 changes: 0 additions & 5 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,6 @@ const styles = {
marginTop: 20,
},

signinLogo: {
height: variables.componentSizeLarge,
width: variables.componentSizeLarge,
},

signinWelcomeScreenshot: {
height: 354,
width: 295,
Expand Down
1 change: 1 addition & 0 deletions src/styles/variables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export default {
fontSizeNormal: 15,
fontSizeLarge: 17,
fontSizeh1: 19,
iconSizeNormal: 20,
mobileResponsiveWidthBreakpoint: 1000,
safeInsertPercentage: 0.7,
sideBarWidth: 300,
Expand Down