Improve Expensicon lib #1188

roryabraham · 2021-01-07T02:51:46Z

Details

This just improves the scalability of the Expensicon lib

Fixed Issues

https://github.com/Expensify/Expensify/issues/151818

Tests

Just regression test each platform to make sure that .svg assets still work as they should.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2021-01-07T02:51:59Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

src/components/Expensicons/Icons.js

src/pages/signin/SignInPageLayout/SignInPageLayoutNarrow.js

src/styles/variables.js

roryabraham · 2021-01-07T21:49:53Z

Okay, this is ready for another round of review. I separated out brand assets from Expensicons.

nickmurray47

LGTM and tests well

marcaaron

Looks great overall. Just have a few style notes and minor ideas.

src/components/Icon/BRAND_ASSETS.js

src/components/Icon/EXPENSICONS.js

src/components/Icon/index.js

marcaaron · 2021-01-07T22:49:50Z

src/components/Icon/index.js

+const ICONS = _.extend(BRAND_ASSETS, EXPENSICONS);
+
+const propTypes = {
+    icon: PropTypes.oneOf(_.values(ICONS)).isRequired,


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.

src/components/Icon/index.js

marcaaron · 2021-01-07T22:58:48Z

src/components/Icon/index.js

+Icon.propTypes = propTypes;
+Icon.defaultProps = defaultProps;
+
+export default memo(Icon);


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

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.

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?

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:

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.

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.

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

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

shawnborton · 2021-01-08T17:38:55Z

Just to double check here, are have you converted all of the icons into svgs here? I don't think so but wanted to check.

I think the outstanding list is something like:

pencil icon for draft messages
attach icon
send/paper plane icon
close (x) icon

I'm happy to provide all of those assets for you in .svg format if you want to get them into this PR.

tgolen · 2021-01-08T18:01:12Z

Before jumping into the code in this PR, I'd like to take a step back because I think this is missing a problem statement. What is the problem being solved here? Has it been documented and confirmed? Is it enough of a problem that it requires refactoring?

Understanding that will help me to go into the code with the right context, but at this point, I am not sure why these changes are necessary.

roryabraham · 2021-01-09T17:51:48Z

@tgolen So high-level the problem solved by this PR is something like this:

In its current form, the Expensicon library requires that each icon be its own component, despite the fact that every component will have nearly the same props and implementation. This creates a scalability issue, where adding a new icon involves creating a duplicate component unnecessarily.

This PR solves that problem by creating a reusable component for icons of different types. Icon assets are collected into their own file, so adding a new icon is as simple as adding the asset to assets/images, then adding it to the list of icons.

roryabraham · 2021-01-09T17:52:41Z

@shawnborton happy to include all the other assets too - should be pretty simple

shawnborton · 2021-01-09T23:25:41Z

Here ya go: IconsForRory.zip

Julesssss

SVG and reusable component refactor ❤️

Few minor points to discuss

Julesssss · 2021-01-22T10:19:40Z

Any final comments @shawnborton ?

src/components/Icon/BrandAssets.js

src/components/Icon/index.js

# Conflicts: # src/components/CreateMenu.js # src/pages/home/sidebar/SidebarLinks.js

roryabraham · 2021-01-25T20:23:03Z

Okay, simplified and updated to only deal with Expensicons and not brand assets for now.

Improve scalability of Expensicon lib

649898c

roryabraham self-assigned this Jan 7, 2021

roryabraham requested a review from a team as a code owner January 7, 2021 02:51

botify requested review from nickmurray47 and removed request for a team January 7, 2021 02:52

shawnborton reviewed Jan 7, 2021

View reviewed changes

src/components/Expensicons/Icons.js Outdated Show resolved Hide resolved

shawnborton reviewed Jan 7, 2021

View reviewed changes

src/pages/signin/SignInPageLayout/SignInPageLayoutNarrow.js Outdated Show resolved Hide resolved

shawnborton reviewed Jan 7, 2021

View reviewed changes

src/styles/variables.js Outdated Show resolved Hide resolved

roryabraham added 3 commits January 7, 2021 09:09

rename iconSizeSmall to iconSizeNormal

4c88a0c

remove unused style

07732e1

Separate brand assets from expensicons, export components not strings

ee93a19

roryabraham requested a review from marcaaron January 7, 2021 21:50

nickmurray47 previously approved these changes Jan 7, 2021

View reviewed changes

marcaaron previously requested changes Jan 7, 2021

View reviewed changes

roryabraham added 3 commits January 7, 2021 15:14

Clean up exports

6fa3400

Separate imports from Icon module

7fdc2d1

Use named exports instead of defaults

17f1bfb

roryabraham dismissed nickmurray47’s stale review via 17f1bfb January 7, 2021 23:34

roryabraham added 2 commits January 7, 2021 15:38

Use PascalCase instead of UPPER_SNAKE

2b2f70f

add props docs

0b2c146

roryabraham requested a review from marcaaron January 7, 2021 23:45

roryabraham added 2 commits January 9, 2021 15:57

include more svg assets

125d18f

Use svg icons throughout app

81ed7b0

Julesssss reviewed Jan 21, 2021

View reviewed changes

roryabraham added 3 commits January 21, 2021 13:16

Get rid of isEnabled prop

09f1452

Simplify fill color logic

d76e860

fix fill color logic

e129576

roryabraham requested a review from Julesssss January 21, 2021 21:27

Julesssss previously approved these changes Jan 22, 2021

View reviewed changes

tgolen requested changes Jan 22, 2021

View reviewed changes

src/components/Icon/BrandAssets.js Outdated Show resolved Hide resolved

src/components/Icon/index.js Outdated Show resolved Hide resolved

src/components/Icon/index.js Outdated Show resolved Hide resolved

src/components/Icon/index.js Outdated Show resolved Hide resolved

shawnborton previously approved these changes Jan 22, 2021

View reviewed changes

roryabraham added 4 commits January 24, 2021 09:50

Fix prop comments

7415d5f

rename icon prop to src

4a0e4b1

Fix AnimatedIcon icon prop

df026f9

Merge branch 'master' into Rory-ImproveExpensicons

6ee53f0

# Conflicts: # src/components/CreateMenu.js # src/pages/home/sidebar/SidebarLinks.js

roryabraham dismissed stale reviews from shawnborton and Julesssss via 6ee53f0 January 24, 2021 18:02

roryabraham requested a review from tgolen January 24, 2021 18:03

Make more icon size variables

a6f4ca8

roryabraham requested a review from shawnborton January 24, 2021 18:15

shawnborton previously approved these changes Jan 25, 2021

View reviewed changes

Julesssss previously approved these changes Jan 25, 2021

View reviewed changes

Remove brand assets file and simplify fill logic

e6a7727

roryabraham dismissed stale reviews from Julesssss and shawnborton via e6a7727 January 25, 2021 20:22

tgolen approved these changes Jan 25, 2021

View reviewed changes

nickmurray47 approved these changes Jan 25, 2021

View reviewed changes

shawnborton approved these changes Jan 26, 2021

View reviewed changes

marcaaron merged commit 6424cfd into master Jan 26, 2021

marcaaron deleted the Rory-ImproveExpensicons branch January 26, 2021 18:45

github-actions bot locked and limited conversation to collaborators Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Expensicon lib #1188

Improve Expensicon lib #1188

roryabraham commented Jan 7, 2021 •

edited

Loading

github-actions bot commented Jan 7, 2021 •

edited

Loading

roryabraham commented Jan 7, 2021

nickmurray47 left a comment

marcaaron left a comment

marcaaron Jan 7, 2021

marcaaron Jan 7, 2021

roryabraham Jan 7, 2021

marcaaron Jan 8, 2021

roryabraham Jan 8, 2021

marcaaron Jan 8, 2021

marcaaron Jan 8, 2021

roryabraham Jan 8, 2021

shawnborton commented Jan 8, 2021

tgolen commented Jan 8, 2021

roryabraham commented Jan 9, 2021

roryabraham commented Jan 9, 2021

shawnborton commented Jan 9, 2021

Julesssss left a comment

Julesssss commented Jan 22, 2021

roryabraham commented Jan 25, 2021

Improve Expensicon lib #1188

Improve Expensicon lib #1188

Conversation

roryabraham commented Jan 7, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Jan 7, 2021 • edited Loading

roryabraham commented Jan 7, 2021

nickmurray47 left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnborton commented Jan 8, 2021

tgolen commented Jan 8, 2021

roryabraham commented Jan 9, 2021

roryabraham commented Jan 9, 2021

shawnborton commented Jan 9, 2021

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented Jan 22, 2021

roryabraham commented Jan 25, 2021

roryabraham commented Jan 7, 2021 •

edited

Loading

github-actions bot commented Jan 7, 2021 •

edited

Loading