-
-
Notifications
You must be signed in to change notification settings - Fork 2
팀 소개 섹션 개발 #681
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
base: main
Are you sure you want to change the base?
팀 소개 섹션 개발 #681
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@Hecklebot 님, PR이 아직 Draft 상태이지만 코드를 보면 구현 작업은 거의 끝나신 것 같네요. 본 PR이 병합되는데 얼마나 시간이 걸릴 것 같으신가요? 올해가 가기 전에 마케팅 페이지 홍보가 가능할지 파악하기 위해 여쭙니다. |
|
@DaleSeo 님. 제가 수, 목요일에는 작업이 어려워서 오늘 중에 마무리해보겠습니다! 병합은 리뷰 상태에 따라 빠르면 금요일, 늦어도 일요일에는 진행되면 좋겠습니다. |
Bundle ReportChanges will increase total bundle size by 23.88kB (4.31%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: daleui-bundle-esmAssets Changed:
Files in
|
DaleSeo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@Hecklebot 안녕하세요 한샘님 디자인쪽 수정이 발생하였는데 누락될까봐 링크 남깁니다. |
| <div | ||
| className={css({ | ||
| position: "absolute", | ||
| left: "50%", | ||
| top: "50%", | ||
| transform: "translate(-50%, -50%)", | ||
| color: "#5cb85c", | ||
| })} | ||
| > | ||
| <Icon name="codeXml" size="md" /> | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ce2a735 에서 figma에 적용된 svg를 추가해두었습니다.
|
@Hecklebot 님, #681 (review) 에서 말씀드린 문제가 여전히 해결이 안 된 것으로 보입니다. |
|
@DaleSeo 님. 제 윈도우 환경에서는 현재 이렇게 보이고 있습니다. 스토리북 Docs에선 실제 너비와 반응형 브레이크 포인트가 일치하지 않아 설정을 추가해 실제 너비에 맞는 반응형 레이아웃을 그리도록 설정해 두었습니다. 맥/사파리 환경의 문제인지 퇴근해서 다시 확인해보겠습니다. |
src/marketing/Team.tsx
Outdated
| import akaAvatar from "../assets/images/marketing/team/members/aka.webp"; | ||
| import daleAvatar from "../assets/images/marketing/team/members/dale.webp"; | ||
| import eunjiAvatar from "../assets/images/marketing/team/members/eunji.webp"; | ||
| import evanAvatar from "../assets/images/marketing/team/members/evan.webp"; | ||
| import hansaemAvatar from "../assets/images/marketing/team/members/hansaem.webp"; | ||
| import helenaAvatar from "../assets/images/marketing/team/members/helena.webp"; | ||
| import hyoseongAvatar from "../assets/images/marketing/team/members/hyoseong.webp"; | ||
| import riaAvatar from "../assets/images/marketing/team/members/ria.webp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인 사진을 저장소에 저장하는 방식을 다시 한 번 고려해 주셨으면 좋겠습니다. 새로운 팀원이 합류할 때 마다 개인 사진을 요청해야하고, 기존 팀원이 사진을 바꾸고 싶을 때는 Pull Request가 필요하기 때문에 유지보수가 번거로워질 것 같아요.
디스코드에서도 말씀드렸듯이 GitHub의 프로필에서 충분히 가져올 수 있는 사진들입니다. 예를 들어, 제 프로필 사진은 아래 URL로 접근할 수 있습니다.
https://avatars.githubusercontent.com/u/5466341?v=4
본인 사진을 사용하고 싶지 않다는 팀원 분들께서는 이미 깃허브 프로필에도 본인 사진이 아니기 때문에 큰 문제되지 않을 거라고 생각합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
달레님께서 제안주신 것에 동의합니다. 멤버별 사진을 포함하면 유지보수 측면에서 부담이 클 것 같아요. 깃헙 프로필 이미지 url을 사용하는 방향이 더 유연하고 관리하기 쉬울 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 해당 방식으로 변경하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5f237bb 에서 수정했습니다!
hyoseong1994
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사진 저장한건 제거하고 링크 방향으로 가는거 같아서 저는 추가 코멘트 없습니다
고생하셨습니다!
|
@jj5u 님 linkedIn light 이미지 비율 변경했습니다. |
@Hecklebot 님, 저는 #681 (review) 에서 Marketing Page의 스토리에 대해서 문제를 보고드렸었는데, 한샘님께서는 Team의 스토리를 보고 계신 것 같습니다. 방금 다시 확인해봤는데 여전히 문제가 해결되지 않았습니다.
|
| import githubLightIcon from "../assets/images/marketing/team/github-light.webp"; | ||
| import githubDarkIcon from "../assets/images/marketing/team/github-dark.webp"; | ||
| import linkedInLightIcon from "../assets/images/marketing/team/linkedIn-light.webp"; | ||
| import linkedInDarkIcon from "../assets/images/marketing/team/linkedIn-dark.webp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Icon 컴포넌트를 쓰지 않고, 생 이미지를 쓰는 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 iconography에 브랜드 아이콘에 createBrandIcon 함수를 이용해 fill="currentColor"를 적용하고 있는데, 이걸 적용하지 않는 아이콘을 또 export 하는 게 맞는지 고민이 되었습니다.
또한 Figma에서 사용 중인 아이콘과 iconography에 추가된 svg 아이콘의 크기가 달라 Icon 컴포넌트에 className을 통한 스타일링을 적용해야 하는데, #491 에서 추가하지 않는 방향으로 논의가 진행되어서 그 부분도 고려했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hecklebot 아하, 그런 고민이 있으셨군요! 아이콘 컴포넌트의 API를 수정하는 방향으로 풀 수도 있었던 문제일 것 같은데, 이제와서 관련 논의를 시작하기에는 너무 늦은감이 있네요. 앞으로는 이렇게 일관성에 타협을 하셔야하는 상황에서는 차선책에 대해서 좀 더 적극적으로 팀 차원에서 커뮤니케이션 해주시면 좋을 것 같습니다.
@jj5u 님, 피그마 디자인을 보니 애초에 아이콘 컴포넌트를 쓰지 않으신 것 같네요. 디자인 단부터 일회성 스타일을 지양하고 최대한 디자인시스템에서 제공하는 컴포넌트를 활용하는 것이 매우 중요하다고 생각합니다. 그렇지 않으면 개발 과정에서 이렇게 임의의 구현 결정을 하게 됩니다. 가급적 이러한 결정은 시스템 단에서 이미 되어 있어서 개발자가 고민할 필요가 없는 것이 디자인시스템의 목표일 것입니다.






변경 사항
리뷰어에게
PR 작성자 체크 리스트