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

Feat: Footer Component #46

Merged
merged 5 commits into from
Nov 9, 2024
Merged

Feat: Footer Component #46

merged 5 commits into from
Nov 9, 2024

Conversation

Sunjae95
Copy link
Contributor

@Sunjae95 Sunjae95 commented Nov 2, 2024

체크리스트

  • 이슈가 연결되어 있나요?

@Sunjae95 Sunjae95 self-assigned this Nov 2, 2024
@Sunjae95 Sunjae95 linked an issue Nov 2, 2024 that may be closed by this pull request
@Sunjae95
Copy link
Contributor Author

Sunjae95 commented Nov 2, 2024

작업중에 의존성있는 작업을 제외하고 구현했습니다.
회의 및 대화로 정해야할 것

  1. 메뉴 Text, URL
  2. svg파일 assets 폴더로 이관하여 사용할 것인가?

@Sunjae95 Sunjae95 marked this pull request as ready for review November 2, 2024 15:31
@Sunjae95 Sunjae95 requested a review from a team as a code owner November 2, 2024 15:31
@DaleSeo
Copy link
Contributor

DaleSeo commented Nov 2, 2024

작업중에 의존성있는 작업을 제외하고 구현했습니다. 회의 및 대화로 정해야할 것

  1. 메뉴 Text, URL
  2. svg파일 assets 폴더로 이관하여 사용할 것인가?

회의에서 논의하고 싶은 내용은 미리 안건 #38 에 추가해주시면 회의 시간을 더 효율적으로 활용할 수 있을 것 같습니다.

Copy link
Contributor

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

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

잘 구현하셨습니다! Footer를 각 페이지 컴포넌트에 연결을 해주셔야 비로서 요건이 만족했다고 볼 수 있을 것 같습니다.

src/components/Footer/Footer.tsx Outdated Show resolved Hide resolved
<ul className={rightMenuStyle}>
{rightMenu.map(({ label, link, component }, i) => (
<li key={link + i}>
<a href={link} target="_blank" aria-label={label}>
Copy link
Contributor

Choose a reason for hiding this comment

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

a11y 👍

Copy link
Member

@sounmind sounmind left a comment

Choose a reason for hiding this comment

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

추억이 새록새록한 Footer 코드... 고생하셨습니다.

Comment on lines 29 to 36
const memberItems = screen.getAllByRole("listitem");
const memberItems = within(
screen.getByRole("region", { name: /members list/i }),
).getAllByRole("listitem");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

footer에 list가 포함되어 test케이스가 실패했습니다.
그래서 within으로 자식 dom을 바라보도록 테스트케이스를 수정했는데 테스트코드로 자주 사용하는 전략일까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

within()으로 요소 검색 범위를 줄이는 거 아주 흔히 사용되는 테스트 작성 패턴입니다.

@Sunjae95
Copy link
Contributor Author

Sunjae95 commented Nov 5, 2024

#50
작업 이후에 다시 수정하겠습니다.

@DaleSeo
Copy link
Contributor

DaleSeo commented Nov 5, 2024

#50
작업 이후에 다시 수정하겠습니다.

@Sunjae95 방금 #50 병합하였습니다.

image

src/components/Certificate/Certificate.tsx Show resolved Hide resolved
width: 80%;
}

> ul:nth-child(1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

HTML 요소에 불필요한 클래스 달지 않고 CSS 쫌 하시네요 👍

width: 80%;
}

> ul:nth-child(1) {
Copy link
Member

@sounmind sounmind Nov 7, 2024

Choose a reason for hiding this comment

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

(변경사항 요청 아님) 저는 개인적으로 nth-child 사용을 지양하려고 하는 편인데요. 스타일 코드를 읽을 때 의미를 바로 파악하기 어려워서였습니다. "ul의 첫번째 자식, 두 번째 자식이 어떤 역할인 거지?"하고 소스 코드를 왔다갔다 하면서 혼란스러워 할 것 같아요. 진짜 디자인 요구사항이 그냥 의미 없이 첫 번째 두 번째 엘리먼트에 특정 스타일을 적용하는 것이라면 어쩔 수 없겠지만요...

Copy link
Contributor

@DaleSeo DaleSeo Nov 7, 2024

Choose a reason for hiding this comment

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

@sounmind 충분히 일리가 있는 의견이라고 생각해요. 저랑 상반되는 피드백이니 가급적 대안 접근까지 제시해주시면 선재님께서 혼란이 덜 할 것 같습니다 :)

Copy link
Member

Choose a reason for hiding this comment

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

그리고 지금 Footer.module.css 의 구성을 봤을 때, 모든 스타일이 footer 클래스 아래에 다 들어가 있는데요.
depth가 4개는 넘어보이고, 아래쪽 스타일을 읽다보면, 어디 단위의 p 태그를 말하는지 헷갈립니다😅
이런 방식의 스타일 작성은 일반 css 파일 하나 작성한 다음, Footer 컴포넌트 상위에 불러오는 것과 무엇이 다른 것인지 모르겠습니다. 적당한 단위로 스타일을 나눠 depth를 줄이고, CSS Modules의 특징을 활용해보는 것은 어떨까요..?
아래 예시는 참고용입니다.

<footer className={styles.footer}>
  <section className={styles.section}>
    <ul className={styles["left-links"]}>
      {leftMenu.map(({ label, link }) => (
        <li key={link}>
          <a href={link} target="_blank" aria-label={label}>
            {label}
          </a>
        </li>
      ))}
    </ul>
    <ul className={styles["right-links"]}>
      {rightMenu.map(({ label, link, component }) => (
        <li key={link}>
          <a href={link} target="_blank" aria-label={label}>
            {component}
          </a>
        </li>
      ))}
    </ul>
  </section>
  <p>© 2024 DaleStudy. All rights reserved.</p>
</footer>

Copy link
Contributor

@DaleSeo DaleSeo Nov 7, 2024

Choose a reason for hiding this comment

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

아마도 @Sunjae95 님께서 #50 (comment) 을 읽으시고 CSS Nesting을 활용하시려다보니 이렇게 스타일이 나오지 않으셨나 추측되네요.

저는 HTML과 CSS 간의 depth 일치해서 나란히 두고 코드를 읽으면 편하던데 개발자마다 다르게 느낄 수 있는 것 같습니다. 저는 @sounmind 님께서 제안하신 것처럼 적당한 지점에서 스타일을 나누는 방식에도 이견은 없습니다. 단, 한 가지 고려할 점은 우리가 지금 Top-down 방향으로 컴포넌트화를 시키고 있기 때문에, 나중에 이 컴포넌트의 많은 부분이 자식 컴포넌트로 스타일과 함께 빠져나갈 수 있다는 겁니다. 그러면 자연스럽게 depth는 줄 것입니다. 따라서 depth가 지나치게 많아지는 것을 컴포넌트 화가 필요한 신호로 볼 수도 있을 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sounmind @DaleSeo 엇 제가 코멘트에 답변달려고 고민하던 사이에 많은일이 있었군요. PR담당자인데 사라진점 죄송합니다😅
저의 의도는 css nesting을 적극 사용하는것이고, 작성하면서 @sounmind 님과 같이 가독성이 떨어지는구나 생각도 들었습니다.
textlink같은 태그는 컴포넌트화하여 분리하면 depth가 줄여질텐데 그걸 말로 표현하기가 어려워서 늦어졌는데 @DaleSeo 님께서 잘 표현해주셨습니다.

제 생각엔 가독성이 힘들었던 부분은 media query가 들어가게되어 블록이 추가되고 child을 읽기가 힘들어졌다고 예상돼요.
또한 현재 PR에서는 앞으로 textLink가 분리되기에 depth가 더 깊어질일은 없다고 생각하구요.

.footer {
  background-color: var(--text-900);
  padding: 80px 27px;

  > section {
    display: flex;
    flex-direction: column;
    gap: 40px;

    > ul:nth-child(1) {
      display: flex;
      flex-direction: column;
      row-gap: 10px;
      line-height: 20px;
      /* component화 할 것*/
      > li > a {
        font-size: 16px;
        color: var(--bg-100);
      }
    }

    > ul:nth-child(2) {
      display: flex;
      align-items: center;
      column-gap: 40px;
    }
  }

  > p {
    font-size: 14px;
    color: rgba(253, 253, 255, 0.75);
    border-top: 1px solid rgba(253, 253, 255, 0.2);
    margin-top: 26px;
    padding-top: 20px;
    line-height: 18px;
  }
}

@media (min-width: 768px) {
  .footer {
    display: flex;
    flex-direction: column;
    align-items: center;
    padding-top: 180px;
    padding-bottom: 100px;

    & > section {
      justify-content: space-between;
      flex-direction: row;
      width: 80%;

      & > ul:nth-child(1) {
        flex-direction: row;
        column-gap: 80px;
      }
    }

    & > p {
      width: 80%;
      margin-top: 50px;
      padding-top: 48px;
    }
  }
}

위는 예시코드인데 depth가 4 depth까지가는걸 방지하고 media query를 분리하여 작성하면 가독성면에서 괜찮을까요??

Copy link
Member

@sounmind sounmind Nov 8, 2024

Choose a reason for hiding this comment

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

같이 통화하면서 작성했던 예제 코드

.footer {
  background-color: var(--text-900);
  padding: 80px 27px;

  @media (min-width: 768px) {
    display: flex;
    flex-direction: column;
    align-items: center;
    padding-top: 180px;
    padding-bottom: 100px;
  }

  section {
    display: flex;
    flex-direction: column;
    gap: 40px;

    @media (min-width: 768px) {
      justify-content: space-between;
      flex-direction: row;
      width: 80%;
    }
  }

  p {
    font-size: 14px;
    color: rgba(253, 253, 255, 0.75);
    border-top: 1px solid rgba(253, 253, 255, 0.2);
    margin-top: 26px;
    padding-top: 20px;
    line-height: 18px;

    @media (min-width: 768px) {
      width: 80%;
      margin-top: 50px;
      padding-top: 48px;
    }
  }
}

.leftMenu {
  display: flex;
  flex-direction: column;
  row-gap: 10px;
  line-height: 20px;

  @media (min-width: 768px) {
    flex-direction: row;
    column-gap: 80px;
  }

  > li > a {
    font-size: 16px;
    color: var(--bg-100);
  }
}

.rightMenu {
  display: flex;
  align-items: center;
  column-gap: 40px;
}
import styles from "./Footer.module.css";

export default function Footer() {
  return (
    <footer className={styles.footer}>
      <section>
        <ul className={styles.leftMenu}>
          {leftMenu.map(({ label, link }) => (
            <li key={link}>
              <a href={link} target="_blank" aria-label={label}>
                {label}
              </a>
            </li>
          ))}
        </ul>

        <ul className={styles.rightMenu}>
          {rightMenu.map(({ label, link, component }) => (
            <li key={link}>
              <a href={link} target="_blank" aria-label={label}>
                {component}
              </a>
            </li>
          ))}
        </ul>
      </section>

      <p>© 2024 DaleStudy. All rights reserved.</p>
    </footer>
  );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@sounmind @Sunjae95 대화를 통해 좋은 타협안을 찾으신 것 같군요 :) 미디어 쿼리가 안으로 들어가서 스타일이 읽기 편한 것 같습니다 💯

@Sunjae95 Sunjae95 merged commit 7edfe09 into main Nov 9, 2024
1 check passed
@Sunjae95 Sunjae95 deleted the 42-footer-component branch November 9, 2024 04:12
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.

Footer Component
3 participants