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

KDT5_YoonGeumYeop #71

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

Conversation

DevYBecca
Copy link

안녕하세요.
패스트캠퍼스 FE 5기 윤금엽입니다.
과제 링크와 설명을 포함한 README.md 파일도 함께 업로드 하였습니다!

Copy link

@chanuuuuu chanuuuuu left a comment

Choose a reason for hiding this comment

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

SCSS, 번들러까지 사용해보시느라 고생하셨습니다.👍🏻

자신만의 방식으로 구현하셨기에 클론코딩을 통해 많은 것을 배우셨을 것 같아요. 더나아가 조금 더 생각해볼만한 것들을 코멘트 드렸습니다.

bundler 사용이 처음이라 배포과정 중 오류가 계속 발생하여 어려웠습니다..!

## 작업한/ 아쉬운 부분
- 요소들을 float로 작업한 실제 사이트와 다르게 flex로 정렬하고 position(relative, absolute)로 추가배치 했습니다.

Choose a reason for hiding this comment

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

기존 구현 그대로가 아닌 flex와 postion을 이용한 구현을 적용해보는 것이 큰 도움이 될 것 같습니다👍🏻

Copy link
Author

Choose a reason for hiding this comment

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

현업에서 실무를 할 때에는 flex와 position을 함께 사용하여 정렬 및 배치를 하는 경우가 많은지 개인적으로 궁금합니다! flex로 원하는 레이아웃 배치가 된다면 좋겠지만 그렇지 못한 경우가 많았기 때문에 추가적으로 position을 사용하였는데 코드가 간결하고 깔끔하다는 느낌을 받지 못해서 의구심이 들기도 했습니다..!

Choose a reason for hiding this comment

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

현업에서 사용하는 방식이 천차만별이나, position만 사용하더라도 충분히 구현이 가능합니다. 다만 연습하는 과정이므로 둘 모두 사용해보는 것이 좋을 것 같아요:)

README.md Outdated Show resolved Hide resolved
dist/index.html Outdated
<!-- Browser Style Reset -->
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/reset.min.css">
<link rel="stylesheet" href="/main.39afc03c.css">
<script src="/main.39afc03c.js"></script></head>

Choose a reason for hiding this comment

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

</head>가 들여쓰기가 되어있지 않습니다. 추후 IDE를 통해 HTML 계층 구조에 맞게 들여쓰기 하시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

과제 수행 당시 bundler에 대해 잘 알지 못해 실제 배포 파일은 dist 폴더 내에 생성된다는 점을 몰랐습니다! index.html의 경우 들여쓰기가 맞게 되어있지만, dist/index.html의 경우 오류가 있었던 것 같습니다..!

Choose a reason for hiding this comment

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

아하 dist 내부의 파일이라 lint가 적용되지 않았던 것이군요:)

dist/index.html Outdated Show resolved Hide resolved
dist/index.html Outdated Show resolved Hide resolved
dist/index.html Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

레파지토리 외부말고 dist내에 또 index.html이 존재하는 이유가 있을까요?

제 생각엔 dist 디렉토리 하위의 파일은 빌드 과정에서 발생하는 파일들이므로 자동으로 생겨나고 있는 것으로 보입니다. 이러한 파일들은 빌드/배포시에만 필요하기 때문에 .gitignore을 통해 형상관리 대상에서 제외하는 것을 추천드립니다!

Copy link
Author

@DevYBecca DevYBecca Apr 20, 2023

Choose a reason for hiding this comment

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

bundler를 처음 사용하다 보니 시행착오가 있었던 것 같습니다. node_modules 폴더는 .gitignore를 통해 제외하였는데, 이번 과제처럼 배포가 필요하지 않은 경우엔 dist 폴더 전체를 버전관리 대상에서 제외하는 것이 좋을까요?

Choose a reason for hiding this comment

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

네, build하는 과정에서 dist가 자동 생성되기 때문에 버전관리 대상에서 제외하셔도 좋습니다.

main.scss Show resolved Hide resolved
main.scss Show resolved Hide resolved
Copy link
Author

@DevYBecca DevYBecca left a comment

Choose a reason for hiding this comment

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

멘토님 리뷰 참고하여 리팩토링 진행하였습니다.
리팩토링 시 수정/ 추가된 사항은 README.md 파일에 작성하였습니다!


<!-- CDN Web Fonts - Nanum Barun Gothic -->
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/nanumbarungothic.min.css">
<!-- Browser Style Reset -->

Choose a reason for hiding this comment

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

전체적인 줄바꿈을 통해서 가독성이 더욱 높아진것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

이전에 prettier를 사용하다가, 초반 강의를 보고 IDE 프로그램을 바꿨었는데 특강 때 강사님께서 prettier를 권장하셔서 재설치 후 리팩토링을 하다보니 전체적으로 줄바꿈이 된 것 같습니다! 감사합니다:)

<!-- MAIN -->
<main>
<section class="container">
<section class="main__title">

Choose a reason for hiding this comment

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

section class = "container"안에 다시 section class="main_title"으로 묶어주셨는데 container로 따로 묶으신 이유가 궁금합니다!

Copy link
Author

@DevYBecca DevYBecca Apr 24, 2023

Choose a reason for hiding this comment

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

main>section.container<main>의 전체적인 컨텐츠를 아우르는 container 역할로 잡았고, section.container의 자식요소로 총 3개의 section.main__title, section.main__info, article.main__notice로 섹션을 나누어 작업했습니다:)!

Copy link

@dmswl2030 dmswl2030 left a comment

Choose a reason for hiding this comment

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

전체적으로 레이아웃도 목적에 맞게 잘나누시고 시멘틱태그 활용, 클래스명도 BEM으로 작성하셔서 너무 깔끔하다고 생각합니다! 고생많으셧습니다~

<img src="./images/logo.png" alt="BGF복지재단" />
<dl class="info">
<dt>주소 :</dt>
<dd>서울특별시 종로구 동숭3길 26-12 2층 (우: 03085)</dd>

Choose a reason for hiding this comment

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

dl, dt, dd 태그들을 사용하신 이유가 있으실까요??

Copy link
Author

Choose a reason for hiding this comment

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

실제 사이트에서 <dl>, <dt>, <dd>태그를 사용하여 작업한 것을 참고하였습니다! <div><span>태그로 작업해도 문제는 없으나, 주소 : 서울특별시~ 이 형태가 정의를 할 때 사용하는 <dl>태그 중 {key:value}형태로 정보를 제공할 때 사용할 수 있다고 알고 있습니다! 그렇기 때문에 적합한 태그 사용이라고 생각하여 그대로 사용하게 되었습니다:)

</li>
<li>
<a href="javascript:void(0)">
<span class="contents__title--ellipsis">

Choose a reason for hiding this comment

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

믹스인을 통해서 구현한 방식에 대해서 궁금합니다! modifier로 추가하게 되면, 사전에 믹스인으로 설정되었던 스타일이 반영되는 방식인가요?

Copy link
Author

Choose a reason for hiding this comment

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

1차 mixin

여러 개의 중복되는 span.contents__title 중 말줄임표를 넣고 싶은 요소에게만 modifier로 --ellipsis를 추가 하였습니다! scss 초반에 작성한 common 스타일 다음으로 @mixin 스타일을 작성하고 mixin의 이름을 'ellipsis'로 붙여 주었습니다. span.contents__title--ellipsis에서 mixin의 스타일을 구현하기 위해 추가적으로 필요한 스타일을 더 작성하고, @include로 mixin을 호출하는 방식으로 사용 해보았습니다!

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.

4 participants