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

fix: 사이트 이름 클릭 시 사이트로 이동하게 수정 #293

Closed
wants to merge 5 commits into from

Conversation

ssolfa
Copy link
Collaborator

@ssolfa ssolfa commented Aug 28, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

2024-08-28.5.28.04.mov
  • handleExtractDomain 함수는 서버에서 받아온 link ("link": "https://fun.ssu.ac.kr/ko/program/all/view/1584",)에서 상위 도메인만 추출하여 ResultListItem 컴포넌트로 보냅니다.

    • URL의 프로토콜의 상위 도메인을 반환하는 origin 속성을 사용하였습니다. 관련 문서
  • handleDomainClick 함수는 도메인을 새 창에서 여는 역할을 합니다. ResultListItem 전체에 클릭 이벤트가 이미 적용되어 있기 때문에 이벤트 전파를 막아 StyledDomain 내에서만 상위 도메인 링크가 열리도록 하고 그 외의 영역에서는 기존에 전달된 링크가 열리도록 설정했습니다.

  • 클릭이 되는 것이라는 걸(??) 알 수 있도록 마우스 호버시 underline이 생기게 하는 효과를 적용하였습니다.

  • resolved [FIX]: 검색 결과에서 사이트 이름 클릭 시 사이트로 이동하지 않는 문제 #266

2️⃣ 알아두시면 좋아요!

  • 혹시 저렇게 상위 도메인을 추출하는 방법 말고 다른 방법이 있다면,,, 알려주세여
  • 현재 서버에서 받아오는 링크가 펀시스템밖에 없는데 서버 오류인지 원래 그런건지 모르겠습니다

3️⃣ 추후 작업

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

@nijuy
Copy link
Collaborator

nijuy commented Aug 28, 2024

어라
여기만 리뷰어 배정이 안 돌아가네요
당황

Copy link
Member

@2wndrhs 2wndrhs left a comment

Choose a reason for hiding this comment

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

현재로서는 지금처럼 URL에서 Origin을 추출하는 방법이 최선인 것 같네요

근데 API 응답보면 source: '펀시스템'이 와서 백엔드에서 sourceLink도 응답으로 주면 좋겠다..

@@ -32,12 +32,22 @@ export const ResultList = () => {
const { data: currentUser } = useGetUserData();
const userId = currentUser?.email || '';

const handleExtractDomain = (url: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

리액트에서 handle~로 시작하는 네이밍은 이벤트 핸들러에 사용되는 것이 일반적이니 handle을 제거하고 함수가 하는 일을 명확하게 드러낼 수 있도록 extractOrigin 정도로 함수 이름을 바꾸는 것이 어떨까요?

또 리액트 의존성 없이 범용적으로 사용할 수 있는 함수이니 위치를 utils 폴더 아래로 옮겨도 좋을 것 같네요

Copy link
Collaborator

Choose a reason for hiding this comment

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

천잰가?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오... 👍👍👍👍👍 적극반영 하였습니다!!!

refactor: 상위 도메인 추출함수를 utils로 분리 및 ResultList에 적용

const handleDomainClick = (e: React.MouseEvent) => {
e.stopPropagation();
if (domain) {
window.open(domain, '_blank');
Copy link
Member

Choose a reason for hiding this comment

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

window.opentarget 속성은 기본값이 _blank라네요

#289 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

사실 저 pr 보고 호버 썼는데... 리뷰를 안봤네요,,,,,,,,,,,,,,,,, 수정했습니다!!

refactor: window.open()에서 기본값인 _blank 제거

Copy link
Collaborator

@nijuy nijuy left a comment

Choose a reason for hiding this comment

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

코드 수정은 이미 준이 잘 짚어줘서! 넘어가겠습니다 굿코드 어썸코드.....

저희가 QA 할 때 실제 요구사항과 다르게 구현된 부분요구사항은 충족하지만 개선이 필요해보이는 부분을 모두 이슈로 만들었잖아요! 저는 이 PR이 후자로 이해되는데 맞나요? 어떤 의도로 만든 이슈인지 검색 QA를 맡았던 쏠이 더 정확하게 알 거 같아 물어봅니다

후자가 맞다는 가정하에~ 마저 코멘트를 적어보자면
요구사항에 없던 기능을 웹에서 추가할 거면 반드시 연관된 다른 팀과 논의를 거친 후에 진행하는 게 적절해보여용

왜냐면 웹에서 기능을 추가할거면 모바일 앱에서도 동일한 경험을 제공해야 하는 거 아닌가? 하는 생각이 들어서요!
그리고 이 기능을 넣게 된다면 사실 각 플랫폼에서 extractOrigin을 만드는 것보다 백엔드에서 링크를 보내주는 게 더 안전하단 생각이 듭니다

이랬는데 전자면 웃기겠다. 암튼 제 생각은 그렇습니다.........

그리고 제가 다른 팀과 논의해서 진행 여부 결정해야 할 거 같은 이슈는 처음부터 QA 할 때 안내했어야 하는데 저의 실수............................ 송죄합니다 흑흑

@2wndrhs
Copy link
Member

2wndrhs commented Aug 29, 2024

그러네요 놓치고 있던 부분이었는데 보리가 잘 지적해준 것 같습니다👍

모든 플랫폼에서 동일한 사용자 경험을 제공해야 한다는 것에 동의하고 QA 과정에 각 플랫폼에서 다르게 구현되어 있는 기능을 통일하는 작업이 포함되어야 할 것 같네요.

이번 변경 사항은 웹에서만 추가한 기능이니 보리가 말한 것처럼 다른 팀에 의견을 묻는 과정이 필요할 것 같네요!

@ssolfa
Copy link
Collaborator Author

ssolfa commented Sep 1, 2024

저희가 QA 할 때 실제 요구사항과 다르게 구현된 부분요구사항은 충족하지만 개선이 필요해보이는 부분을 모두 이슈로 만들었잖아요! 저는 이 PR이 후자로 이해되는데 맞나요?

일단 후자는 맞습니다!

왜냐면 웹에서 기능을 추가할거면 모바일 앱에서도 동일한 경험을 제공해야 하는 거 아닌가? 하는 생각이 들어서요!
그리고 이 기능을 넣게 된다면 사실 각 플랫폼에서 extractOrigin을 만드는 것보다 백엔드에서 링크를 보내주는 게 더 안전하단 생각이 듭니다

오... 완전 동의합니다

요구사항에 없던 기능을 웹에서 추가할 거면 반드시 연관된 다른 팀과 논의를 거친 후에 진행하는 게 적절해보여용

요구사항에 없는 기능을 추가하는 거라 로깅에도 들어가지 않고 뭔가 끼워넣은 느낌이 들긴 했는데... 그렇다면 보류해두겠습니다!
그리고 다른 팀과 논의를 거치기 전에 웹팀에게도 질문을 하고 싶은데요,,, 검색 qa에서만 이슈를 만든 거라 다른 웹팀분들의 개선 할만 한지에 대한 판단을 알고싶은데 일단 웹팀에게 질문 -> 개선 동의시 다른 팀과 기능 추가 논의 이렇게 넘어가도록 하겠습니다! 추가 논의가 된다면 그 때 백엔드에게 링크 제공 요청도 한 번 해보는 걸로,,,?!

@2wndrhs
Copy link
Member

2wndrhs commented Sep 1, 2024

검색 기능 디자인할 때 네이버 검색을 많이 참고한 것 같은데 네이버는 검색 결과와 별개로 사이트 이름을 클릭하면 해당 사이트로 이동하네요!

확실히 UX 적으로 좋을 것 같아서 저는 추가하는게 좋을 것 같아요!

@nijuy
Copy link
Collaborator

nijuy commented Sep 1, 2024

다른 팀과 논의를 거치기 전에 웹팀에게도 질문을 하고 싶은데요,,, 검색 qa에서만 이슈를 만든 거라 다른 웹팀분들의 개선 할만 한지에 대한 판단을 알고싶은데 일단 웹팀에게 질문 -> 개선 동의시 다른 팀과 기능 추가 논의 이렇게 넘어가도록 하겠습니다! 추가 논의가 된다면 그 때 백엔드에게 링크 제공 요청도 한 번 해보는 걸로,,,?!

넵 좋은 방법이에요! 그렇게 되면 거쳐야 할 단계가 많아지니,
다음 QA부터는 종료 직후에 후자에 속하는 이슈를 다같이 검토하는 시간을 가져도 좋겠네요
저도 처음이라 놓친 부분이 많았는데 멋찐 의견 감사합니다!!!! 🤩

+) 앞으로 다른 사람의 의견이 필요한 이슈에 대해서는 RFC label을 달아보면 어떨까요?
Request for Comments라는 뜻이고, 해당 이슈 코멘트로 의견 주고 받으면 좋을 거 같아요 (오프라인이 어려울 경우!)
아직 웹팀에서 잘 활성화 되지는 않았지만 🥲 개발 과정에서 진행된 논의는 휘발되지 않는 곳에 남기고 싶어서요

image


저는 이게 꼭 있어야 할 정도의 기능인지는 아직 판단이 잘 안 섭니다!
일단 웹에서는 커서 올리면 밑줄이 쳐지니까 상위 사이트로도 이동 가능하다는 점을 쉽게 알 수 있는데,
앱에서는 그렇지 않으니까 이 기능 자체를 인지하기 어렵진 않을까? 하는 걱정이 가장 큰 이유에요.

또 검색 엔진마다 상위 링크로의 이동을 지원하는 곳도 있고 아닌 곳도 있어서,
다른 사이트에서 축적된 경험에 기대기도 약간.. 어렵쥐 않을까......

@owl1753
Copy link
Collaborator

owl1753 commented Sep 1, 2024

저는 저 사이트 이름 이라는 것의 역할이 어디에서 쓰여진 글인 지를 나타내기 위한 장치로써만 먼저 인지가 되어서, 굳이 저 기능이 필요한가 싶긴 하네요... 다른 분들의 의견도 들어봐야 할 것 같습니다!

@ssolfa
Copy link
Collaborator Author

ssolfa commented Sep 2, 2024

저는 사이트나 source가 다양하다면 상위 사이트로 이동하는 것이 좋다고 생각합니다.
개인적인 경험으로는 있는 게 편하긴 하지만... 숨쉴에서 쓰이는 source가 펀시스템과 숭실대학교 홈페이지 정도밖에 없는 걸로 알아서 사이트 특성을 생각해본다면 없어도 되는 기능이지 않나 싶습니다!

그리고 해당 기능을 정확히 정의하는 명칭을 찾아봤는데 도저히 못 찾겠어서... 혹시 아시는 분 계신가요?!!
구글의 사이트 이름 이런 건 있긴 하더라구요

@nijuy
Copy link
Collaborator

nijuy commented Sep 2, 2024

데이터 소스 늘어나면 좋겠다!는 의견은 계속 나왔으나 실제로 반영이 가능한지, 된다면 추가되는 시점과 개수는 어떤지 저도 잘 파악하고 있지 않아서요..........🤯

제가 화요일 파트리드 회의에서 관련 내용 한번 더 확인해보고 ➡️ 수요일 팀 회의에서 공유할게요
이번 QA에서 꼭 진행해야 할 작업은 아닌 걸로 의견이 모아지는 듯하니 PR은 잠시 닫아두는 쪽으로 할까요?

그리고 해당 기능을 정확히 정의하는 명칭을 찾아봤는데 도저히 못 찾겠어서... 혹시 아시는 분 계신가요?!!

따로 용어가 있지는 않은 거 같은데.... 알게되면 공유하기... 혼자 알고있기 없음 ㅜㅜ!!!!!!!!!

@ssolfa ssolfa closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FIX]: 검색 결과에서 사이트 이름 클릭 시 사이트로 이동하지 않는 문제
4 participants