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: Handle spaces in initialConsonants search inputs properly #42

Merged

Conversation

BO-LIKE-CHICKEN
Copy link
Contributor

@BO-LIKE-CHICKEN BO-LIKE-CHICKEN commented Apr 15, 2024

Overview

#41 에서 발견한 '공백이 포함된 초성 검색 시 예상치 못한 false 반환 문제'를 공백 포함 입력도 정상적으로 처리하도록 수정했습니다.

  • 공백을 포함한 초성 입력시에도 정상적으로 작동하도록 수정
  • 공백을 포함한 초성 입력시에도 정상적으로 작동하는지 체크하기 위한 테스트 케이스 추가

PR Checklist

  • I read and included theses actions below
  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.

Copy link

changeset-bot bot commented Apr 15, 2024

🦋 Changeset detected

Latest commit: 6798078

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for es-hangul ready!

Name Link
🔨 Latest commit 6798078
🔍 Latest deploy log https://app.netlify.com/sites/es-hangul/deploys/6621e374d71d250008d884ff
😎 Deploy Preview https://deploy-preview-42--es-hangul.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

evan-moon
evan-moon previously approved these changes Apr 17, 2024
Copy link
Member

@evan-moon evan-moon left a comment

Choose a reason for hiding this comment

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

LGTM! 고생하셨습니다 🙇

@evan-moon
Copy link
Member

@BO-LIKE-CHICKEN ...는 Conflict가 발생했군요 😢

@BO-LIKE-CHICKEN
Copy link
Contributor Author

BO-LIKE-CHICKEN commented Apr 17, 2024

@evan-moon 충돌을 해결했습니다! #36 과 충돌이 있었네요 🥲 다시 리뷰해주시면 감사하겠습니다!

다만, #41 이슈에서 @okinawaa 님과 함수의 역할에 대해 논의하기 이전의 PR이므로, 병합 시 문제가 될 가능성이 있습니다. 두 리뷰어의 의견을 모아주시면 좋을 것 같아요. 🙏🏻

해당 이슈에 따른 개선안으로도 PR을 생성할 예정입니다.

감사합니다!

return false;
}

const initialConsonantsX = getFirstConsonants(x).replace(/\s/g, '');
const initialConsonantsY = getFirstConsonants(y).replace(/\s/g, '');
const initialConsonantsY = trimY;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

위에서 초성으로만 구성되었는지 확인하기 때문에 getFirstConsonants는 필요없습니다.

@BO-LIKE-CHICKEN
Copy link
Contributor Author

참고사항

  • 빈 배열을 검사하는 함수를 직접 구현해서 추가할지, slash 라이브러리에 있는 isNonEmptyArray등을 추후 라이브러리를 의존성에 추가하는 방향으로 갈지 파악하지 못하여 함수내부에서 처리했습니다.
  • 현재 chosungfirstConsonants가 혼용되어 사용되고 있습니다. (머지가 된다면) 이후 getFirstConsonants부터 일관성있게 워딩을 바꿔가면 좋을 것 같습니다.

감사합니다!

src/chosungIncludes.ts Outdated Show resolved Hide resolved
okinawaa
okinawaa previously approved these changes Apr 19, 2024
Copy link
Member

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

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

참고사항으로 남겨주신 두 가지 의견모두 동의합니다! 넘 좋네용

okinawaa
okinawaa previously approved these changes Apr 19, 2024
Copy link
Member

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

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

리뷰를 너무 늦게해서 죄송합니다
감사해요!

@okinawaa okinawaa merged commit f668e15 into toss:main Apr 19, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Apr 19, 2024
@BO-LIKE-CHICKEN BO-LIKE-CHICKEN deleted the fix/chosung-search-whitespace-issue branch April 19, 2024 08:17
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.

None yet

3 participants