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

Feature/joh 12 footer #2

Merged
merged 13 commits into from
Dec 24, 2023
Merged

Feature/joh 12 footer #2

merged 13 commits into from
Dec 24, 2023

Conversation

MichaelScodaeng
Copy link
Contributor

@MichaelScodaeng MichaelScodaeng commented Dec 20, 2023

  • Creating Footer

Creating Footer
-Font error
- Logo centralization
@boomchanotai
Copy link
Member

  1. รัน pnpm lint:fix หน่อย (lint คือตัวกำหนด convention ของ code มันจะ force เราให้เขียนตาม format ใด format หนึ่งที่กำหนดกฎให้มันไว้)
  2. พยายามอย่ากำหนด width ตาม figma คือใน figma อ่ะ มันบอก width ก็จริง แต่มันสำหรับขนาดหน้าจอใดหน้าจอหนึ่งเท่านั้น เราต้องเขียนให้มัน responsive กับหน้าจอทุกอัน อย่ากำหนด width เยอะ หรือถ้ากำหนดแนะนำให้ใช้เป็น % ดีกว่า
  3. ทำ Responsive สำหรับ Mobile ด้วย
  4. เก็บรูปใน src/assets/ จะสร้าง folder ใหม่ชื่อ footer ก็ได้ แล้วใช้การ import รูปมาใช้แทน เพราะว่า เวลาเรา import มันจะทำให้ Vite เอาไฟล์รูปเข้าไปร่วมคำนวนตอน build javascript ด้วย docs

Vite คือ Bundler ของ Javascript มันคือตัวที่เอา Files JavaScript ของที่เราเขียนมาแยกเป็นส่วน ๆ, จัดการสร้างไฟล์ html เพียวๆให้, แยก styles, css, และจัดการเรียกใช้แต่ละส่วนเมื่อ user เข้าหน้าเว็บมา

Copy link
Member

@boomchanotai boomchanotai left a comment

Choose a reason for hiding this comment

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

ทำตามที่ comment ไป

@MichaelScodaeng
Copy link
Contributor Author

MichaelScodaeng commented Dec 22, 2023

  • Making it prettier with pnpm lint:fix
  • Make it responsive
  • Change most scale from px to % or smth like that
  • You are king GG!
  • logo pictures in add src/assets

return (
<div className="mt-auto w-full">
<div className="relative mx-auto h-[243px] w-full shrink-0 xl:h-[215px]">
<div className="absolute left-0 top-0 h-full w-full bg-teal-600">
Copy link
Member

Choose a reason for hiding this comment

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

หลีกเลี่ยงการใช้ position absolute ถ้าไม่จำเป็น คือมันสามารถจัด layout โดยใช้ flex ได้อ่ะ

<div className="absolute left-0 top-0 h-full w-full bg-teal-600">
<div className="hidden xl:block">
<div
className="absolute left-[1.85%] top-[40.46%] inline-flex h-[19.53%]
Copy link
Member

Choose a reason for hiding this comment

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

นี่ก็ด้วย ลองใช้การจัด layout ด้วย flex แทน

src={johnjudLogo}
alt="Johnjud Logo"
/>
<div className="font-Poppins text-base font-bold text-white">
Copy link
Member

Choose a reason for hiding this comment

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

font-poppins ฝากแก้ทุกอันด้วย

</div>
</div>
{/* Below md*/}
<div className="absolute left-[7.63%] top-[12.20%] flex h-[76.27%] w-[55%] flex-col items-start justify-start gap-4 xl:hidden">
Copy link
Member

Choose a reason for hiding this comment

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

ตอน responsive ใช้ lg: แทน xl: แล้วก็ถ้าเปลี่ยนไปใช้ flex แล้วอ่ะ จะไม่ต้องซ่อน element แล้ว ลอง responsive ด้วย flex ดู

return <div className="mt-auto">Footer</div>;
return (
<div className="mt-auto w-full">
<div className="relative mx-auto h-[215px] w-full shrink-0 lg:h-[215px]">
Copy link
Member

Choose a reason for hiding this comment

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

บรรทัดนี้อ่ะ

  1. relative จะไม่มีผลอะไร เพราะเราไม่ได้ใช้คู่กับ absolute
  2. mx-auto ไม่มีผลอะไรเพราะว่า div เราเต็มหน้าจอ มันจะมีผลคือ ถ้า div เราไม่เต็มจอ มันจะจัดให้อยู่ตรงกลาง
  3. h-[215px] กับ lg:h-[215px] มันไม่ได้เปลี่ยน responsive อะไรเลย

บรรทัดนี้ลบ class ไปหมดเลยก็ได้ แล้วไปกำหนด padding py-.. บรรทัดต่อไปแทน

return (
<div className="mt-auto w-full">
<div className="relative mx-auto h-[215px] w-full shrink-0 lg:h-[215px]">
<div className="relative flex h-full w-full justify-between bg-teal-600 px-[7.63%] py-[6.80%] lg:px-[1.85%] lg:py-[0%]">
Copy link
Member

Choose a reason for hiding this comment

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

ตรงนี้สามารถกำหนด padding สำหรับ หน้าจอ desktop ได้ แทนการใช้ height
ก็คือลบ height แล้วเปลี่ยนไปใช้ padding แทนให้หมด
แล้วก็ padding อ่ะ ใช้เป็น px ไม่ก็ rem ได้ ใช้เป็น % ไม่เวิร์คเท่า px หรือ rem แต่จริงๆแนะนำให้ลองกะๆห้มันประมาณ docs tailwind ดู พวก px-5 px-6 px-8 อ่ะ

พยายามอย่าใช้ height มาก ถ้าสมมุติว่ามันสามารถจัดโดยให้มันแปรผันตาม element (ข้อความรูปภาพที่อยู่ข้างใน)

</div>
{/*Text*/}
<div className="inline-flex flex-col gap-4 lg:w-[458px] lg:gap-3">
<div className="w-full p-0 text-xs font-normal leading-[15px] text-white lg:text-[20px] lg:leading-[25px]">
Copy link
Member

Choose a reason for hiding this comment

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

อันนี้ฝากเปลี่ยน lg:text-base หน่อย คิดว่าใช้ font ขนาดธรรมดาน่าจะดูดีกว่า ใน figma ทำไมมันดูสมส่วนก็ไม่รู้

Copy link
Member

Choose a reason for hiding this comment

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

จริงๆ leading ไม่ต้องก็ได้นะ พวก class text-base text-xs มันทำให้หมดละ

</div>
</div>
{/*Social*/}
<div className="inline-flex flex-col items-center justify-center gap-6">
Copy link
Member

Choose a reason for hiding this comment

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

สงสัยว่าทำไมใช้ inline-flex

Copy link
Member

@boomchanotai boomchanotai left a comment

Choose a reason for hiding this comment

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

Perfect!

@MichaelScodaeng MichaelScodaeng merged commit 3767efb into dev Dec 24, 2023
1 check passed
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