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

chore: update text content and author in participant review component #179

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

yolophg
Copy link
Contributor

@yolophg yolophg commented Jul 23, 2024

Checklist before merging

  • Link an issue with the pull request
  • Ensure no errors or warnings on the browser console
  • Avoid additional major pushes after approval (if necessary, request a new review)

Sorry, something went wrong.

@yolophg yolophg linked an issue Jul 23, 2024 that may be closed by this pull request
@yolophg
Copy link
Contributor Author

yolophg commented Jul 24, 2024

@DaleSeo @nhistory
달레, 세환님, 참가자들의 실제 리뷰로 replace 하고난 뒤, 데스크톱, 태블릿 사이즈에서 디자인의 변화가 생겨서 함께 논의 후 수정해야 할 듯 하여 두분의 의견도 여쭈어봅니다. 최대한 자세히 설명 드리긴 했는데 혹시, 이해가 안가시는 부분이 있으면 오늘 미팅에서 물어봐주셔도 좋을 것 같습니다!
아래 영상은 replace 하고 난 이후의 영상입니다. 보시면, 각자 리뷰의 길이가 모두 달라서, 디자인 상에서 기존에 정했던 각 스크린 사이즈 별 height 보다 더 늘어나게 되며, 레이아웃이 좀 흐트러지게 됩니다.
데스크톱)
https://github.com/user-attachments/assets/d8dc6d05-4a44-4045-93c6-b4a004b20984
태블릿)
https://github.com/user-attachments/assets/1421f965-bd1d-4a6d-a9f2-d58d8db46ca1

그래서, height를 고정하여 scroll 처리나, ellipsis 처리를 하는 방법도 있고, 리뷰의 내용이 제공되는 것도 중요하지 않을까 판단 되긴 해서 어떠한 길이도 수용하는 방향으로 가야하는게 맞을지 하여 이와 관련한 두분의 의견을 여쭙니다.
참고로, 동영상 내에서 보이고 있는 참가자 리뷰의 순서는 리뷰 내용 길이를 딱히 고려하지 않고, 우선은 참가 리뷰를 올려주신 순서대로 나열되고 있는 상태입니다.
위 질문을 정리하면, 아래와 같습니다.

  • height를 고정?
  • 어떠한 길이도 수용?

추가적으로, 어떠한 길이도 수용하는 방향으로 갈 시, 현재 participant review component에서 수정해야 할 부분도 생길 것 같아 이와 관련해서도 고민인데요.

  • 위 영상을 보시면 아실 수 있겠지만, 데스크톱 사이즈일 때, 참가자 이미지리뷰 내용 이 중앙 정렬이 되어야 합니다.
    기존에 명시된 해당 컴포넌트 height 기준으로는, 참가자 이미지 를 align-items: flex-start로 해줄 때 중앙정렬이 맞았던 상태입니다.
    왜냐하면, 태블릿 사이즈일 때 참가자 이름 이 reverse 되어야해서 지금 컴포넌트 구조 상 리뷰 내용참가자 이름같은 컨테이너 로 묶여 있는 상태이기 때문입니다. 이 구조와 관련해서 더 디테일한 내용은 아래 피그마를 참고해주시면 됩니다.
    https://www.figma.com/design/j4gGV8VkvpOCL1UYpP99rt?node-id=98-443#842789284
  • 그래서, 구조를 다시 참가자 이미지리뷰 내용 을 묶는 것으로 바꾸고, 태블릿 사이즈일 때는 아래에 배치되고 있던 참가자 이름 을 display none 해주고 위에 배치된 또 다른 참가자 이름 엘리먼트 를 보여주는 방식으로 가거나, 아니면 현재 구조에서 참가자 이미지 위 padding을 수동으로 추가해주는 방식으로 수정하거나 해야할 것 같습니다.

@nhistory
Copy link
Contributor

@yolophg 자세한 코멘트 감사드립니다.

우선 제가 생각했을 때 가장 간단한 해결방법은 <blockquote> 부분의 height을 고정값으로 주는 방법인데요.

  • 가장 긴 content를 기준으로 height 값을 고정해서 주는 방법
  • 인접한 2개 섹션 중 더 많은 값을 기준으로 height을 해당하는 2개 섹션에만 주는 방법

위 2가지 중 하나를 채택해보면 어떨까 합니다.

그리고 또 한가지 생각해본 방법은 원래 figma 디자인 상의 height 값을 고정으로 주고 더보기 버튼으로 가려진 텍스트를 보여줬다가 다시 숨겼다가 하는 방법도 가능할 것 같습니다. (toggle button)

@yolophg
Copy link
Contributor Author

yolophg commented Jul 24, 2024

@nhistory 의견 감사드립니다! 제가 생각할 때는 더보기를 사용하는 방식이 기존 디자인도 잘 유지하면서 내용도 잘 제공할 수 있는 가장 좋은 방법이라고 생각되긴 하는데요. 어쨌든 디자인이랑 개발 둘다 리소스가 좀 더 들어가긴 해야하니 마지막 이터레이션이었어서 조금 고민이 되었습니다.
달레님만 괜찮으시면 저는 더보기를 추가하는 방식이 좋을 것 같다고 생각되긴 합니다!
일단은, 세환님께서 제안 주신 두가지 방법으로 먼저 제가 적용해서 한 번 테스트 해보겠습니다!

@DaleSeo DaleSeo mentioned this pull request Jul 25, 2024
5 tasks
@DaleSeo
Copy link
Contributor

DaleSeo commented Jul 28, 2024

@yolophg 요거 작업이 언제 쯤 끝날 예정이실까요? 오늘 스터디 1기를 차주에 마무리하기로 해서, 차주에 2기 모집을 위해서 웹사이트를 써야할 것 같아서요 ㅋ

@yolophg
Copy link
Contributor Author

yolophg commented Jul 28, 2024

@yolophg 요거 작업이 언제 쯤 끝날 예정이실까요? 오늘 스터디 1기를 차주에 마무리하기로 해서, 차주에 2기 모집을 위해서 웹사이트를 써야할 것 같아서요 ㅋ

아, 그렇군요! 제가 오늘이랑 내일 작업할 예정이라, 마무리하고 세환님이랑도 한 번 더 컨펌하면 늦어도 월욜에는 마무리 할 수 있을 것 같아요!
이번 이터레이션이 수욜까지니까 그 전에는 무조건 마무리 될 듯 한데, 혹시 조금 더 빠른 마무리가 필요하실까요?
말씀주신 차주에 2기 모집이, 혹시 월욜부터 모집을 시작할 수 있는 상태여야할까요?

@DaleSeo
Copy link
Contributor

DaleSeo commented Jul 28, 2024

@yolophg 원래 생각하시던 일정으로 진행하셔도 될 것 같습니당! 차주에 종훈님께서 1기 종료 Shout-out 글을 먼저 올리시고, 제가 2기 모집글을 그 다음에 올리기로 했거든요.

@yolophg
Copy link
Contributor Author

yolophg commented Jul 28, 2024

@yolophg 원래 생각하시던 일정으로 진행하셔도 될 것 같습니당! 차주에 종훈님께서 1기 종료 Shout-out 글을 먼저 올리시고, 제가 2기 모집글을 그 다음에 올리기로 했거든요.

네, 참고하겠습니다.
빨리 마무리 될수록 좋을 것 같으니, 우선순위 올려서 빠르게 마무리 해보겠습니다!

@yolophg
Copy link
Contributor Author

yolophg commented Jul 29, 2024

@nhistory 님, height 수정해서 최종적으로 변경된 사항 레코딩입니다! 확인 부탁드립니다 :)
human 아이콘들도 추가로 반영되어있습니다!

데스크톱)
https://github.com/user-attachments/assets/27e01848-0b5c-4739-8c7f-97f81784f554

태블릿)
https://github.com/user-attachments/assets/f02764e8-22b3-40bd-8095-761a2190b838

@nhistory
Copy link
Contributor

@yolophg 넵 잘 수정된 것 같습니다. 수고하셨어요!

@DaleSeo
Copy link
Contributor

DaleSeo commented Jul 29, 2024

@yolophg 고생 많으셨어요! 그런데 다들 홈페이지에 본인 프로필 사진 쓰시기를 거절하셨나봐요? ㅋㅋ

@yolophg
Copy link
Contributor Author

yolophg commented Jul 29, 2024

@yolophg 고생 많으셨어요! 그런데 다들 홈페이지에 본인 프로필 사진 쓰시기를 거절하셨나봐요? ㅋㅋ

아니요, 별도로 확인은 안해봤는데 모두 깃허브 프로필이 사람 얼굴도 있고 아닌 경우도 있어서 제각각이라 차라리 캐릭터로 통일 되는게 나을 것 같다는 개인적인 의견이 첨가되었습니다 ㅋㅋ😆 저희 스터디 채널에서 태그해서 한 번 여쭤봐볼게용

이 스터디에는 이미 해외에 계신분들도 있으시고, 현재 해외에 계신 분들도 있으셔서 해외 취업과 관련된 많은 정보를 듣고 이야기 나눌 수 있어서 좋았습니다. <br/>
다른 사람들과 함께 스터디 하기 때문에 혼자서 할 때보다 알고리즘을 꾸준히 풀어나갈 수 있도록 동기 부여가 되는 부분이 좋았고, 서로의 작성한 코드를 확인하며 피드백 할 수 있다는 점이 좋았습니다. <br/>
해외 취업을 준비하시는 분들께 좋은 기회가 될 것 이라고 생각합니다."
author="Jonghoon Park"
Copy link
Contributor

Choose a reason for hiding this comment

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

@yolophg @nhistory 이름에 깃허브나 링크드인 프로필 링크 걸어주는 거에 대해서 어떻게 생각하시나용?

Copy link
Contributor

Choose a reason for hiding this comment

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

리뷰 남겨주신 분들이 동의하신다면 저는 좋을것 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 것 같아요! 지금 논의중인 스터디 채널 내 '리뷰 개인 프로필 사진' 쓰레드에서 링크 추가 원하시는 분들 대상으로만 링킹할게요.

Copy link
Contributor

Choose a reason for hiding this comment

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

이 건 제가 즉석해서 건의드린 거니까 따로 이슈를 생성해서 follow-up하는 게 좋을 것 같네요. 😅

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.

고생하셨습니다!

@yolophg yolophg marked this pull request as ready for review July 30, 2024 13:53
@yolophg yolophg requested a review from a team as a code owner July 30, 2024 13:53
@yolophg yolophg merged commit 0113f99 into main Jul 30, 2024
@yolophg yolophg deleted the 175-replace-placeholder-testimonies-with-acutal-ones branch July 30, 2024 13:53
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.

Replace placeholder testimonies with acutal ones
4 participants