-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Added lazy loading of images for both TS and JS supported components #599
Conversation
@@ -2,6 +2,7 @@ | |||
import styled from '@emotion/styled'; | |||
import * as C from '../../ignitus-Helpers/emotion-Styles/colors'; | |||
import * as F from '../../ignitus-Helpers/emotion-Styles/font'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
@@ -9,6 +9,7 @@ import isEqual from 'lodash/isEqual'; | |||
import loader from '../../ignitus-Assets/Images/loader2.gif'; | |||
import * as t from './Constants'; | |||
import { withErrorBoundary } from '../../ignitus-Internals'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
@@ -4,6 +4,7 @@ import React from 'react'; | |||
import { Link } from 'react-router-dom'; | |||
import * as t from './Constants'; | |||
import { withErrorBoundary } from '../../ignitus-Internals'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
@@ -40,7 +41,7 @@ const Introduction = () => ( | |||
</button> | |||
</Link> | |||
</div> | |||
<img className="img-fluid" src={t.resume} alt="resume" /> | |||
<LazyLoadImage className="img-fluid" src={t.resume} alt="resume" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple spaces found before 'className' no-multi-spaces
@@ -3,6 +3,7 @@ import { Link } from 'react-router-dom'; | |||
import '../Styles/style.scss'; | |||
import { withErrorBoundary } from '../../ignitus-Internals'; | |||
import * as t from './Constants'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
@@ -103,7 +104,7 @@ const PureFooter = ({ | |||
<div className="col-md-2 mx-auto pd-left"> | |||
<ul className="list-unstyled"> | |||
<li> | |||
<img src={logo} className="img-responsive img-css" alt="logo" /> | |||
<LazyLoadImage src={logo} className="img-responsive img-css" alt="logo" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected indentation of 14 space characters but found 16 react/jsx-indent
Multiple spaces found before 'src' no-multi-spaces
@@ -7,6 +7,7 @@ import { HashLink } from 'react-router-hash-link'; | |||
import { Link } from 'react-router-dom'; | |||
import { withErrorBoundary } from '../../ignitus-Internals'; | |||
import { logo } from './Constants'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
@@ -8,6 +8,7 @@ import { Redirect, Link } from 'react-router-dom'; | |||
import { HashLink } from 'react-router-hash-link'; | |||
import logo from '../../ignitus-Assets/Images/nav-logo.svg'; | |||
import { withErrorBoundary } from '../../ignitus-Internals'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
@@ -1,6 +1,7 @@ | |||
import React from 'react'; | |||
import { Link } from 'react-router-dom'; | |||
import { withErrorBoundary } from '../../ignitus-Internals'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
@@ -1,6 +1,7 @@ | |||
import React from 'react'; | |||
import { Link } from 'react-router-dom'; | |||
import { withErrorBoundary } from '../../ignitus-Internals'; | |||
import { LazyLoadImage } from 'react-lazy-load-image-component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute imports should come before relative imports import/first
Codecov Report
@@ Coverage Diff @@
## master #599 +/- ##
=============================================
Coverage 18.95425% 18.95425%
=============================================
Files 76 76
Lines 612 612
Branches 129 129
=============================================
Hits 116 116
Misses 496 496
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhu-A-J Your work looks good to me, I will have a through look in evening, although page insights is not complaining about images now, here is the report, https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Fdeploy-preview-599--peaceful-nightingale-55a1b7.netlify.com%2F&tab=desktop
The problem that I see now this PR has effected the speed index, like this is the report that don't have changes in this PR - s https://developers.google.com/speed/pagespeed/insights/?url=http%3A%2F%2Fwww.ignitus.org%2F&tab=desktop
As per bundlephobia I think npm package has effected the time to load by some ms, https://bundlephobia.com/[email protected].
Not sure can you investigate further :) cc @debck , @gittysachin .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @abhu-A-J. But the time of first-input-delay has increased, not every time but sometimes when the browser will deal heavy loads then it will affect the response to any clicked link or button pressed. @divyanshu-rawat it's because of the react-lazy-load-image-component
package. And this npm package is the reason for slightly increased load time as well.
@gittysachin there is improvement in |
@divyanshu-rawat @gittysachin my two cents on this matter.
The images used all all format aren't really compressed that well, a lot of improvements can be done. Here's what I achieved on manual compression.
|
@divyanshu-rawat It's showing completely different result now. Last time when I checked, it was showing results as I previously described. It's continuously changing for me as I refresh the results. |
@abhu-A-J thanks a lot sound good, but now question arises how should we compress the used images? I think the current format is png. I know one to compress One solution is to have an component to render
export default Icon; And the font-awesome thing you mentioned #479 address that. |
I also tried it, results seems to be different one opening it again. |
@abhu-A-J sorry to say, Here I think cost of module is more in comparison to that of value, we are closing it for now keeping in mind the merge conflicts as well. :) thanks for your efforts we really appreciate it, Hope you might have learned some stuff while working in THIS PR 👍 We are removing dependency on font-awesome like this way. https://github.com/Ignitus/Ignitus-client/tree/develop/src/ignitus-Shared/ignitus-DesignSystem/ignitus-Assets/ignitus-Icons Feel free to let us know your views on the same. |
@divyanshu-rawat Ah! Feels bad that this couldn't make a difference to the repo, but no issues I understand your point. I'll look towards other issues and hopefully try to contribute in a better way. Loving the experience btw!! |
fixes #583
Implemented the lazy loading of images concept for both components written in JS and TS.
Breaking Changes: Addition of 2 new npm packages to achieve the same tasks mentioned above.