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

맞춤법 검사 기능 추가 #6

Merged
merged 6 commits into from
Jul 26, 2022
Merged

맞춤법 검사 기능 추가 #6

merged 6 commits into from
Jul 26, 2022

Conversation

rycont
Copy link
Contributor

@rycont rycont commented Jul 21, 2022

부산대 맞춤법검사기 Integration을 추가하였습니다

@rycont
Copy link
Contributor Author

rycont commented Jul 21, 2022

image

Copy link
Owner

@hjh010501 hjh010501 left a comment

Choose a reason for hiding this comment

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

안녕하세요 @rycont 님.
멋진 기능과 함께 제 프로젝트에 기여해 주셔서 감사합니다.
코드 리뷰 결과 몇가지 이슈 사항이 있어서 한번 확인해 주시면 감사하겠습니다 :)

index.html Outdated

document.getElementById("go_spellcheck")?.addEventListener("click", (e) => {
e.target.style.setProperty("opacity", "0.5")
fetch("https://spell.rycont.workers.dev/" + encodeURIComponent(textarea.innerText))
Copy link
Owner

Choose a reason for hiding this comment

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

해당 부분에서 버튼을 클릭하면 개인이 보유하신 cf worker server 로 작성된 내용을 보내는 듯 하는데, 이에 대한 이슈가 몇가지 있습니다.

스크린샷 2022-07-21 오후 8 44 01

  1. 현재 해당 웹사이트는 하루에 7000명이 접속하는 웹 사이트이고, 동시 접속자가 생각보다 많은 사이트 입니다. 보유하신 cf worker 로 많은 요청이 갈 수 있을 듯 한데요, 이에 대한 비용 발생 문제가 생길 가능성이 있어 보이는데 확인 부탁드립니다!
  2. 사이트로 요청을 보내게 될 경우, modal 등을 이용하여 이용자에게 입력 데이터가 맞춤법 검사 서버로 가게 된다는 걸 사전 공지해야 할 것 같습니다.
  3. 그리고 @rycont 님이 올려주신 개인 서버가 데이터를 수집하지 않는다는 것을 증명하여야 할 것 같습니다.. (근데 어떻게 증명을 해 주셔야 할지 저도 잘 모르겠습니다 😢)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제 PR에 대해 깊이 고민해주셔서 고맙습니다🙏

  1. Workers 프리티어가 1일 10만회로 여유있는 사용량을 가지고 있어서, 비용 관련해서 문제가 되진 않을 것 같습니다. 맞춤법 검사 기능이 서비스에 적용된다면 사용량 증가에 대비해서 rate limit을 설정해두려고 합니다.

  2. 다만 말씀해주신 것과 같은 정보수집 우려에 대한 문제는 제가 미처 생각하지 못했네요ㅜ 말씀주신 것과 같이 텍스트 정보만 맞춤법 검사기 서버로 전송되며, 내용은 저장되지 않는다는 안내와 함께 기능을 추가하는 편이 맞을 것 같습니다.

2.1. 다만 걱정되는 부분이 한 가지 있는데요, 현재 운용중인 페이지는 사용자의 이용면에서도, 개발용 코드면에서도 매우 단순한 구조를 가지고 있어 직관적으로 이용하고 개발할 수 있다는 점이 가장 큰 장점이라고 생각합니다. 여기에 데이터 전송에 관련된 내용을 안내하는 모달과 같은 추가 요소가 들어간다면, 이와 같은 장점이 희석될 수도 있어 보입니다. 현재 코드에 안내 모달을 추가하는 방안을 허락해 주신다면 진행하도록 하겠습니다! 만약 본 서비스의 테마와 어울리지 않는다면 포크한 제 레포에만 적용해두겠습니다.

Copy link
Owner

Choose a reason for hiding this comment

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

답변해 주셔서 감사합니다!

그러면 한번 적용해 주셔도 좋을 것 같습니다 :)

UX 적인 측면에서 현재 사이트와 큰 차이가 없도록 (맞춤법 검사 버튼을 클릭해야지만 모달이 띄워지게 되고 모달 내에 동의 버튼을 클릭하면 localStorage 에 동의 버튼을 누른 시각을 기록한 후 사용할 수 있도록) 구현해 주시면 정말 감사하겠습니다!

3번의 경우 2번 방법으로 동의를 구하면 해결될 것 같습니다 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네, 현재 페이지의 디자인을 최대한 고수하는 한에서 정보 전송 동의 모달을 구현해서 다시 PR드리겠습니다. 긍정적으로 고려해주셔서 고맙습니다 🤗

@rycont
Copy link
Contributor Author

rycont commented Jul 23, 2022

버튼 클릭 시에 확인 모달이 표시되도록 추가하였습니다. 확인 결과는 localstorage에 저장돼서, 최초 1회만 동의하면 이후에는 모달 창이 뜨지 않습니다.

  • 기존 코드와의 로직 분리를 위해 Webcomponent(lit)을 적용하였습니다.

image

@rycont
Copy link
Contributor Author

rycont commented Jul 23, 2022

  • Lit의 번들 사이즈가 7kb정도로 매우 작아서 사이트 로딩 속도에 거의 영향이 없는 부분을 확인하였습니다.

Copy link
Owner

@hjh010501 hjh010501 left a comment

Choose a reason for hiding this comment

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

멋진 기능 추가해 주셔서 감사합니다!
추가 작업 사항이 없으실 경우 해당 리뷰에 👍 남겨주시면 머지 진행하도록 하겠습니다.
다시 한번 감사드립니다 😊
(좀 예전에 개발한 토이 프로젝트여서 코드가 많이 난잡하였을텐데, 작업해 주셔서 감사합니다 ㅠ)

@rycont
Copy link
Contributor Author

rycont commented Jul 26, 2022

마지막으로 작동 테스트 했습니다! 제 깃헙페이지에 올렸을 때는 잘 됐는데, 프로덕션에 올라가서도 잘 됐으면 좋겠네요.... 머지 부탁드리겠습니다🤗

@hjh010501 hjh010501 merged commit 4b4cde5 into hjh010501:master Jul 26, 2022
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.

2 participants