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

Landing page #2

Open
wants to merge 4 commits into
base: Main
Choose a base branch
from
Open

Landing page #2

wants to merge 4 commits into from

Conversation

MayankKumar10
Copy link
Owner

Adding Header Footer and assets

Copy link

@nayyyhaa nayyyhaa left a comment

Choose a reason for hiding this comment

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

Only homepage is added, so much to comment on. I've mentioned a few pointers you can go through them, the rest of the code looks good.

ForgotPassword,
UserPage,
WishList,
} from "./components/index";

Choose a reason for hiding this comment

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

you can just import from ./components, that /index can be skipped.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, I'll do that.

src="../images/img2.webp"
width="900"
height="700"
alt=""

Choose a reason for hiding this comment

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

try to give alt attribute some description about the image..

Copy link

Choose a reason for hiding this comment

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

For performance basis. You can add --- loading="lazy"------ to img tag. It will help in optimization .

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, I'll do that.

Comment on lines 170 to 178
<div className="mid-card-container sm-card-shadow padding-normal flex-row-center AlertError imgTransition">
<section className="flex-row-start">
<img
className="margin-0 cart-img"
src="../images/pc-svg-1.svg"
alt=""
/>
</section>

Choose a reason for hiding this comment

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

instead of creating card again, you can loop through the card data more for code readability.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, I'll do that. Thank's Neha For PR review

Copy link

@VLeads VLeads left a comment

Choose a reason for hiding this comment

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

I have made few suggestion that you ca look at. Rest all is good.

src="../images/img2.webp"
width="900"
height="700"
alt=""
Copy link

Choose a reason for hiding this comment

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

For performance basis. You can add --- loading="lazy"------ to img tag. It will help in optimization .

Comment on lines 247 to 263
<span
className="card-container card-box cardBadge box-shadow"
data-label="Trending"
>
<section className="img-card-offer padding-normal">
<img
className="imgcard"
src="../images/pc-svg-1.svg"
alt=""
/>
<button className="material-icons-text card-wishlist-icons buttonHoverShadow AvatarImage AvatarIcons flex-row-center icon-wishlist">
<i className="material-icons">
favorite
</i>
</button>
</section>

Copy link

Choose a reason for hiding this comment

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

You can make card component. And use that by using map function

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, I'll do that. Thank's Vishal for PR review

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.

3 participants