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

[SDPA-4520] refactor icon component to improve loading performance #774

Merged

Conversation

rubyllamadora
Copy link
Contributor

@rubyllamadora rubyllamadora commented Sep 9, 2020

JIRA issue: https://digital-engagement.atlassian.net/browse/SDPA-4520

Changed

  1. Refactored icon component to improve loading performance
  2. Converted svg files to use JS for easy manipulation especially for custom icons
  3. Removed assets/img
  4. Added unit test
  5. Updated snapshot to reflect icons

Storybook: https://storybook-ripple-pr-774.lagoon.vicsdp.amazee.io/?path=/story/atoms-icon--icon-library
FE: https://app-ripple-pr-774.lagoon.vicsdp.amazee.io/demo-landing-page

TODO

  • Update dependent websites that has custom icons

Screenshots

backward compatible support

image

Ruby Lamadora added 30 commits July 15, 2020 17:18
@@ -112,7 +115,7 @@ storiesOf('Atoms/Icon', module)
'xls',
'xlsm',
'xlsx',
'youtube',
'youtube_channel',
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @rubyllamadora rename this may will cause issue as I remembered one logic use BE string value to pick youtube icon, rename icon may cause the logic fail.

<% } %>

<% if (options.quickexit) { %>
rplOptions.quickexit = options.quickexit
<% } %>
<% } %>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this indention was added accidentally.

addCustomIcons(require.context('../assets/ripple-icon/', true, /\.svg$/))
<% } %>
// Add custom icons to library.
addCustomIcons(require.context('../assets/ripple-icon/', true, /\.svg$/))
Copy link
Contributor

@tim-yao tim-yao Sep 21, 2020

Choose a reason for hiding this comment

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

So both svg and js custom icons will work for customer site. Will JS one be load automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, JS will be loaded automatically as they are imported in Atoms/Icon/index.js

import './icons/index.js'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tim-yao, I added below require.context which seems to work fine in my local.

ripple: {},
ripple: {
customIcon: true
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This example is nice. Can you please move it to https://github.com/dpc-sdp/ripple/tree/master/examples/basic-examples ?

@tim-yao
Copy link
Contributor

tim-yao commented Oct 6, 2020

300ms saved on homepage local test. 💯
Before:
Screen Shot 2020-10-06 at 6 45 14 pm
After:
Screen Shot 2020-10-06 at 6 45 03 pm

```

Update the name of the icon - this will be used for symbol prop later, `width`, `height`, `paths` and `polygons` accordingly.
Add this to `assets/rpl_icon/index.js`
Copy link
Contributor

Choose a reason for hiding this comment

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

@rubyllamadora Any reason to change the directory name from ripple-icon to rpl_icon ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops. This should be ripple-icon. I'll update.

@@ -172,7 +172,7 @@ Icon.register({
```

Update the name of the icon - this will be used for symbol prop later, `width`, `height`, `paths` and `polygons` accordingly.
Add this to `assets/rpl_icon/index.js`
Add this to `assets/ripple_icon/index.js`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @rubyllamadora ripple-icon instead of ripple_icon? Where is the code to load it? I am afraid we need a test for it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ripple-icon!. I might just revert this README update as this won't be rolled out to clients yet? It will still be svg files for them until we convert them to the new JS format.

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 example I added is inside basic-examples for enabled customIcon. I have JS files created in my local only as I think it is irrelevant to commit it for now. We still need an extra script to process in replacement for addIconsToLibrary() once we roll this out.

@@ -0,0 +1,13 @@
import Icon from '../../../../packages/components/Atoms/Icon/Icon.vue'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll replace this with @dpc-sdp/ripple-icon/Icon.vue once this is good to merge. This is only for this branch to work.

import RplIcon from '../../../packages/components/Atoms/Icon/index.js'
// Imports below will be moved to ripple-nux-ui so we can enable/disable customIcon.
import './../assets/ripple-icon/custom_icon_lock_open'
import './../assets/ripple-icon/custom_icon_search'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tim-yao we just need to import custom icons like this. Below is my test page.
image

Copy link
Contributor

@tim-yao tim-yao left a comment

Choose a reason for hiding this comment

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

Thanks @rubyllamadora !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants