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

Updated the 404 error page with the new design and add the provided Lottie animation. #600

Closed
wants to merge 6 commits into from

Conversation

APOORVA456
Copy link
Contributor

@APOORVA456 APOORVA456 commented Mar 10, 2020

Description

Updated the 404 error page with the new design and add the provided Lottie animation.

Issue Link: #567

Kindly Review it @Paarmita

Screenshots :

Screenshot (98)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@welcome
Copy link

welcome bot commented Mar 10, 2020

Thanks for opening this pull request! Please check out our contributing guidelines.

Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

LGTM @APOORVA456 , Please try to resolve husky suggestions, as it has left some review for you.

@divyanshu-rawat
Copy link
Member

@Paarmita , @gittysachin , @debck please have a look into the PR 👍

Copy link
Member

@gittysachin gittysachin left a comment

Choose a reason for hiding this comment

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

Please resolve hound suggestions as well.

src/ignitus-Routes/Styles/style.scss Outdated Show resolved Hide resolved
Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

Nice work! but I must admit please refrain using px, vw instead use rem in margin, padding, width. #577 was all about this, introducing vw will make it more complex.

src/ignitus-Routes/notFound.js Outdated Show resolved Hide resolved
<Link to="/">
import Lottie from 'react-lottie'
import animationData from '../ignitus-Routes/Styles/404animation.json';
//path to animation file
Copy link

Choose a reason for hiding this comment

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

Expected exception block, space or tab after '//' in comment spaced-comment

src/ignitus-Routes/Styles/style.scss Show resolved Hide resolved
color: {

};
p {
Copy link

Choose a reason for hiding this comment

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

Rule declaration should be preceded by an empty line
Selector should have depth of applicability no greater than 2, but was 3

src/ignitus-Routes/Styles/style.scss Show resolved Hide resolved
src/ignitus-Routes/Styles/style.scss Show resolved Hide resolved
src/ignitus-Routes/Styles/style.scss Show resolved Hide resolved
src/ignitus-Routes/Styles/style.scss Show resolved Hide resolved
src/ignitus-Routes/Styles/style.scss Show resolved Hide resolved
src/ignitus-Routes/Styles/style.scss Show resolved Hide resolved
.container-404 {
min-height: 60vh;
width: 100vw;
width: 100%;
Copy link

Choose a reason for hiding this comment

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

Line should be indented 2 spaces, but was indented 4 spaces

@APOORVA456 APOORVA456 requested a review from gittysachin March 10, 2020 13:56
Copy link
Member

@gittysachin gittysachin left a comment

Choose a reason for hiding this comment

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

Please resolve codacy errors. Everything else looks good.

// color: blue;
color: rgb(10, 33, 151);
font-size: 25px;
a {
Copy link

Choose a reason for hiding this comment

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

Rule declaration should be preceded by an empty line
Selector should have depth of applicability no greater than 2, but was 4

@APOORVA456
Copy link
Contributor Author

Please resolve codacy errors. Everything else looks good.

Okay Sure.

width={500}/>
</div>
</div>
</div>
Copy link

Choose a reason for hiding this comment

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

Expected closing tag to match indentation of opening react/jsx-closing-tag-location
Expected indentation of 3 space characters but found 5 react/jsx-indent

height={400}
width={500}/>
</div>
</div>
Copy link

Choose a reason for hiding this comment

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

Expected closing tag to match indentation of opening react/jsx-closing-tag-location
Expected indentation of 7 space characters but found 6 react/jsx-indent

<Lottie options={defaultOptions}
height={400}
width={500}/>
</div>
Copy link

Choose a reason for hiding this comment

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

Expected closing tag to match indentation of opening react/jsx-closing-tag-location
Expected indentation of 8 space characters but found 6 react/jsx-indent

<div className="animation">
<Lottie options={defaultOptions}
height={400}
width={500}/>
Copy link

Choose a reason for hiding this comment

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

A space is required before closing bracket react/jsx-tag-spacing
Expected indentation of 11 space characters but found 16 react/jsx-indent-props
The closing bracket must be aligned with the line containing the opening tag (expected column 10 on the next line) react/jsx-closing-bracket-location

{/* Lottie Animation */}
<div className="animation">
<Lottie options={defaultOptions}
height={400}
Copy link

Choose a reason for hiding this comment

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

Expected indentation of 11 space characters but found 16 react/jsx-indent-props

<div className="content">
<p ><b>Oops!</b></p>
<p>We can&apos;t seem to find the page you were looking for&nbsp;</p>
<p>
Copy link

Choose a reason for hiding this comment

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

Expected indentation of 10 space characters but found 9 react/jsx-indent

<div className="container-404">
<div className="content">
<p ><b>Oops!</b></p>
<p>We can&apos;t seem to find the page you were looking for&nbsp;</p>
Copy link

Choose a reason for hiding this comment

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

Expected indentation of 10 space characters but found 9 react/jsx-indent

<div>
<div className="container-404">
<div className="content">
<p ><b>Oops!</b></p>
Copy link

Choose a reason for hiding this comment

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

Expected indentation of 10 space characters but found 9 react/jsx-indent

(
<div>
<div className="container-404">
<div className="content">
Copy link

Choose a reason for hiding this comment

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

Expected indentation of 9 space characters but found 8 react/jsx-indent

export const Notfound = () =>
(
<div>
<div className="container-404">
Copy link

Choose a reason for hiding this comment

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

Expected indentation of 5 space characters but found 7 react/jsx-indent

@Paarmita
Copy link
Member

@APOORVA456 Can you please fix the width and height so that the animation does not cut on the sides, it looks like this for me.

Screenshot 2020-03-10 at 8 38 20 PM

Also, it is not responsive enough, Please fix these issues.

Screenshot 2020-03-10 at 8 39 24 PM

@Paarmita
Copy link
Member

@APOORVA456 Also, update your PR with the develop branch and not master #605

Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

Thanks for your work @APOORVA456 thanks a lot, I just realised the it would be great if we put some gif there, react-lottie has severely affected the performance of the app, cost of adding react-lottie is very high see here. https://bundlephobia.com/[email protected] in comparison to that what it is providing what do you think @gittysachin , @debck , @Paarmita .

@divyanshu-rawat
Copy link
Member

Lets find a gif add it there :)

@APOORVA456
Copy link
Contributor Author

Thanks for your work @APOORVA456 thanks a lot, I just realised the it would be great if we put some gif there, react-lottie has severely affected the performance of the app, cost of adding react-lottie is very high see here. https://bundlephobia.com/[email protected] in comparison to that what it is providing what do you think @gittysachin , @debck , @Paarmita .

Yeah..It has affected the performance.

@divyanshu-rawat
Copy link
Member

can you and @Paarmita can look for some gif :) , as I can see lottie animation is continuously running and causing lot 3D rendering.

@divyanshu-rawat
Copy link
Member

divyanshu-rawat commented Mar 11, 2020

Here are some free ones in giphy - https://giphy.com/gifs/404-14uQ3cOFteDaU

@Paarmita
Copy link
Member

Paarmita commented Mar 11, 2020

@APOORVA456 @divyanshu-rawat Leave the idea of GIF as it will also consume space, let's use CSS for this. Here are some examples

We just need to use our theme color and font. Also, add the link to the Home page.

What do you think?

@APOORVA456
Copy link
Contributor Author

@Paarmita I noticed that in second one ,GIF is embedded in CSS so it will rise the same issue as earlier.
I think first one would be better as it's color matches with our theme and and just have to add the home page link.

Copy link
Member

@gittysachin gittysachin left a comment

Choose a reason for hiding this comment

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

@divyanshu-rawat @Paarmita @APOORVA456 let's just leave the react-lottie because it'll reduce the performance of the app. GIFs won't look good and will surely take some extra space. We should go for CSS animation rather than using the GIF.

@divyanshu-rawat
Copy link
Member

@APOORVA456 please go ahead with the 1st one then, and please remove react-lottie as a dependency from your package.json file.

@APOORVA456 APOORVA456 closed this Mar 13, 2020
@APOORVA456 APOORVA456 reopened this Mar 13, 2020
@APOORVA456 APOORVA456 closed this Mar 13, 2020
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.

4 participants