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

vsl 183: lazy loading icons and organize reusable components #102

Merged
merged 13 commits into from
Sep 13, 2022

Conversation

linxiao-li
Copy link
Collaborator

@linxiao-li linxiao-li commented Sep 12, 2022

Description

  1. Implemented dynamic import and lazy loading for svgs,
  • to avoid importing all the svg files into the component when we only need 1 of them, especially for token icons
  • easier to manage and organize

2.Moved Dropdown, ProgressBar and InputAddress into library/components and updated the references

** note: I've also created a set of tickets for implementing storybook as part of the effort in building and maintaining our front-end library https://linear.app/dappercollectives/issue/VSL-195

Demo / Test Result

Before lazy loading:
Webpack load the entire svg file into the bundle:
Screen Shot 2022-09-12 at 11 15 21 AM

After lazy loading:
Webpack load the loading helper Svg/index.js into the bundle
Screen Shot 2022-09-12 at 11 16 40 AM
When we need a svg, we make network requests for the specific file
Screen Shot 2022-09-12 at 11 17 10 AM

@linear
Copy link

linear bot commented Sep 12, 2022

VSL-183

@linxiao-li linxiao-li changed the title Linxiaoli/vsl 183 vsl 183: lazy loading icons and organize reusable components Sep 12, 2022
@@ -1,8 +1,7 @@
import React, { useState } from "react";
import { isEmpty } from "lodash";
import InputAddress from "./InputAddress";
import InputText from "./InputText";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we delete this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

InputAddress is moved to library/InputAddress,
InputText is only used on this page and it performs exactly like a normal text input, no reason to create a separate file for it. On the other hand, if we want to normalize the input behaviors in the future, we can create inputText reusable component and apply it across the board

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be useful to create a new component or modify the one that is deleted so it can also accept things like:

  • label
  • flag - if the field is required (to show the *)
  • placeholder
  • onChange
    That way we will unify the look and have less code written.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

100% agree. Would you mind creating a ticket for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -183,11 +190,16 @@ function SafeContacts({ address }) {
<div className="column p-0 mt-5 is-flex is-align-items-center is-justify-content-space-between is-full">
<h4 className="is-size-5">Saved Addresses</h4>
{!isEmpty(contacts) && (
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

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

Conflict

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated! Thanks a lot!

@@ -53,7 +53,7 @@ const AddCollection = ({ onCancel, onNext }) => {
/>
{addressValid && (
<div style={{ position: "absolute", right: 17, top: 14 }}>
<Check />
<Svg nam="Check" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

@KsenijaZivkovic
Copy link
Collaborator

There are some compiling errors:

  • SafeSettings/components/Owners
  • TransactionSuccesModal
    And one warning related to useEffect in Svg/index.js

Copy link
Contributor

@mannynotfound mannynotfound left a comment

Choose a reason for hiding this comment

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

This is really cool!

I'm working on something similar for CAST that also adds storybook so would be good to re-use this code and maybe even share the same codebase at some point, otherwise this LGTM!

DapperCollectives/CAST#449

Comment on lines +7 to +12
try {
const svgComponent = await import(`./${name}`);
setSvgComponent(svgComponent.default(props));
} catch (e) {
console.log(e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try {
const svgComponent = await import(`./${name}`);
setSvgComponent(svgComponent.default(props));
} catch (e) {
console.log(e);
}
await import(`./${name}`)
.then(({ default: Svg }) => setSvgComponent(<Svg {...props} />))
.catch(console.error)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we should use mixed syntax, it causes confusion and makes the code harder to maintain, this answer from stack overflow explains it well: https://stackoverflow.com/a/54387912.

})();
//eslint-disable-next-line
}, [name]);
if (!svgComponent) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

svgComponent should already be null if component fails to load so this line shouldn't be necessary

packages/client/src/library/Svg/index.js Outdated Show resolved Hide resolved
@linxiao-li
Copy link
Collaborator Author

linxiao-li commented Sep 13, 2022

@mannynotfound Thanks a lot! Does the design of CAST look similar to VESSEL? If so we should promote @cast/shared-components to @dapperCollectives/shared-components, it would save VESSEL a ton of development work reinventing the wheels! @amyliu6

@linxiao-li linxiao-li merged commit 26edf05 into main Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants