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

Single product page #22

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

alirezawdx
Copy link
Collaborator

No description provided.

@netlify
Copy link

netlify bot commented Jun 22, 2023

Deploy Preview for simple-shop-technullogy ready!

Name Link
🔨 Latest commit cc30da7
🔍 Latest deploy log https://app.netlify.com/sites/simple-shop-technullogy/deploys/6520146f9d0f940008324b41
😎 Deploy Preview https://deploy-preview-22--simple-shop-technullogy.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AmirHosseinKarimi
Copy link
Member

  • They should be buttons
  • The border should be implemented by CSS not image
  • Plus and minus icon can be image (SVG)
  • They SHOULD NOT BE ABSOLUTE! Use flex instead.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

AmirHosseinKarimi commented Jul 24, 2023

Use the correct color same as the design.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

AmirHosseinKarimi commented Jul 24, 2023

According to the design, Bullets should be square, not circular.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

Thumbnails height is not same as the design.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

  • The gap between items is not same as design
  • Set min-width to items

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

Its the dot (.) in design. Not the virgule (،). But use the comma (,)

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

Its semi-bold in design.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

Tabs should be clickable. Like as button.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

Its bold in the design.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

Fix bullets size & space same as design.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@AmirHosseinKarimi
Copy link
Member

Products should be linked to their pages.

image

Browser metadata
Path:      /src/pages/single-product.html
Browser:   Firefox 116.0 on Linux x86_64
Viewport:  2560 x 1283 @1x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

Copy link
Member

@AmirHosseinKarimi AmirHosseinKarimi left a comment

Choose a reason for hiding this comment

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

Good job @alirezawdx

I just reviewed your PR and it's awesome. Please take a look at the comments that I placed and apply them. Then request a review.

<meta charset="UTF-8" />
<link rel="icon" type="image/svg+xml" href="/vite.svg" />

<!-- Shbnam font -->
<link href="https://cdn.jsdelivr.net/gh/rastikerdar/shabnam-font@v[X.Y.Z]/dist/font-face.css" rel="stylesheet" type="text/css" />
Copy link
Member

Choose a reason for hiding this comment

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

Fix the font href. Place the correct version in the template section.

Comment on lines +11 to +15
<style>
body {
font-family: 'Shabnam', sans-serif;
}
</style>
Copy link
Member

Choose a reason for hiding this comment

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

It is unnecessary. Look at TailwindCSS documents to set the default font in the config.

</head>
<body>
</head>
<body dir="rtl" class="overflow-x-hidden">
Copy link
Member

Choose a reason for hiding this comment

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

  • It's better to apply direction to the <HTML> tag.
  • It's not a good idea to prevent horizontal scroll by overflow-x-hidden. Instead, fix the issue which causes scroll.
Suggested change
<body dir="rtl" class="overflow-x-hidden">
<body>

<ul class="flex justify-start text-[#7E8AAB] text-[17px] px-[30px] py-[39px]">
<li class="p-0 m-0 flex items-center">
صفحه اصلی
<img src="./../assets/images/icons/arrow-left.png" alt="Arrow Icon" class="w-[5.83px] h-[11.67px] mx-[14px]">
Copy link
Member

@AmirHosseinKarimi AmirHosseinKarimi Jul 24, 2023

Choose a reason for hiding this comment

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

  • Do not force image width & height same time. It may break the image aspect ratio. Just set the width or height.
  • Do not use decimal numbers with pixel unit. Just round it up or down.
  • You can use the nearest TailwindCSS class if there is a small difference.


<!-- navigations -->
<ul class="flex justify-start text-[#7E8AAB] text-[17px] px-[30px] py-[39px]">
<li class="p-0 m-0 flex items-center">
Copy link
Member

Choose a reason for hiding this comment

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

Remove p-0 m-0. Just apply the correct class to the parent. Check the TailwindCSS documents.

</li>
</ul>

<hr class="bg-[#DEE2EEB2] mt-[48px]">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<hr class="bg-[#DEE2EEB2] mt-[48px]">
<hr class="bg-[#DEE2EEB2] mt-12">

Comment on lines +162 to +164
<button class="flex flex-row gap-2 justify-center items-center bg-white border py-3 px-4">
<span class="rounded-full bg-blue-600 w-[15px] h-[15px] inline-block"></span> <span>آبی</span>
</button>
Copy link
Member

Choose a reason for hiding this comment

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

Use <i> for elements like icons, bullets and etc.

Always beautify your code.

Suggested change
<button class="flex flex-row gap-2 justify-center items-center bg-white border py-3 px-4">
<span class="rounded-full bg-blue-600 w-[15px] h-[15px] inline-block"></span> <span>آبی</span>
</button>
<button class="flex flex-row gap-2 justify-center items-center bg-white border py-3 px-4">
<i class="rounded-full bg-blue-600 w-[15px] h-[15px] inline-block"></i>
<span>آبی</span>
</button>

Comment on lines +179 to +181
<button class="flex flex-row gap-2 justify-center items-center bg-white border py-3 px-4">
<span>۱۰×۱۵</span>
</button>
Copy link
Member

Choose a reason for hiding this comment

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

The inner <span> element is unnecessary. You can just remove it and everything works like a charm.

Suggested change
<button class="flex flex-row gap-2 justify-center items-center bg-white border py-3 px-4">
<span>۱۰×۱۵</span>
</button>
<button class="flex flex-row gap-2 justify-center items-center bg-white border py-3 px-4">۱۰×۱۵</button>

Comment on lines +339 to +341
<div class="flex justify-center items-center h-[60px] w-[60px] bg-[#F5F8FC]">
<img src="../assets/images/icons/arrow-right-line.svg" alt="Arrow Icon" class="opacity-60 rotate-180">
</div>
Copy link
Member

Choose a reason for hiding this comment

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

It's clickable elements. So, It should be a button or defined as a button via role=button.

Comment on lines +386 to +387
</body>
</html>
Copy link
Member

Choose a reason for hiding this comment

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

Set up your IDE in a proper way. A blank like at the end of the file is a usual and default configuration. Looks like your IDE does not follow this convention.

Configure your code formatter tool (Prettier) in a proper way that follows the team/project configuration to not override other members' code without any actual change.

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.

2 participants