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

일기장 [STEP 1] Yetti, idinaloq #118

Merged
merged 19 commits into from
Sep 1, 2023

Conversation

idinaloq
Copy link

@havilog
하비 안녕하세요~ 첫 PR 드립니다!

이번 프로젝트동안 잘 부탁드립니다. 🫡

고민한 점

DateFormatter 확장해서 사용하기

DateFormatter를 사용할 때 여러 ViewController에서 사용되고 또 각각의 ViewController마다 다른 날짜를 표시해줘야 했기 때문에 어떤식으로 작성해야할지 고민했습니다. 저희는 extension으로 DateFormatter를 확장하고 각각의 날짜를 호출할 수 있도록 기능을 구현해 주었습니다.

편집중인 텍스트가 키보드에 의해 가리지 않도록 하기

  • 키보드가 나왔을 때, 텍스트가 가려지지 않도록 하는 방법을 몇 가지 알아보았습니다. 공통적으로 NotificationCenter를 사용하는 방법이 있었는데, 그 이유는 keyboardWillShowNotification, keyboardWillHideNotification을 사용해서 키보드가 나타나고 사라질 때 notification을 post 하기 때문입니다.
  • 그 다음으로는 키보드가 나타날 때 textView의 크기 조절을 어떻게 하는지에 따라서도 방법이 나뉘어져 있었습니다. 키보드의 크기에 맞게 텍스트뷰의 위치를 위로 올렸으나, 이렇게 되면 safe area를 벗어나게 되므로 이 방법 대신 content와 contentView 가장자리 간격을 확장하는 contentInset, verticalScrollIndicatorInsets을 사용해서 글자를 가리지 않도록 했습니다.

조언을 얻고 싶은 점

중복되는 코드

현재 DiaryDetailViewControllerNewDiaryViewController에서 키보드 구현을 위한 코드가 작성되어 있습니다. 하지만 중복되는 코드이기에 키보드를 사용하는 UIViewController에 protocol을 채택하였으나 @objc 메서드때문에 사용할 수 없었고, extension을 사용한 경우 새로운 textView를 받아와야하고 각 메서드마다 textView를 받아야하는 것 같은데 그 부분에서 실행이 되지 않아 실패했습니다.
setUpKeyboardEvent(), keyboardWillShow(_ notification: Notification), keyboardWillHide() 이 3개의 메서드를 키보드를 사용하는 뷰컨에서만 사용할 수 있도록 코드를 합치려면 어떤 방식을 사용해야 좋을까요?

swiftLint

  • 키보드가 텍스트를 가리지 않게 하기위해 처음에 scrollIndicatorInsets를 사용했습니다. 하지만 뒤늦게 deprecated 되어있는 것을 발견했는데, 이 때 deprecated 경고가 발생하지 않았습니다.
  • swiftLint Reference에는 관련 부분이 없는것 같은데, 이 부분이 swiftLint와 관련이 있는것인지 아니면 다른 이유가 있는지 궁금합니다.

Copy link

@havilog havilog left a comment

Choose a reason for hiding this comment

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

중복되는 코드의 경우 프로토콜로 정의한 뒤
프로토콜의 extenstion을 활용하는건 어떨까요?

Diary/Controller/DiaryDetailViewController.swift Outdated Show resolved Hide resolved
Diary/Controller/DiaryDetailViewController.swift Outdated Show resolved Hide resolved
Diary/Controller/DiaryDetailViewController.swift Outdated Show resolved Hide resolved
Diary/Controller/DiaryListViewController.swift Outdated Show resolved Hide resolved
Diary/Controller/DiaryListViewController.swift Outdated Show resolved Hide resolved
Diary/Controller/DiaryListViewController.swift Outdated Show resolved Hide resolved
Diary/Extension/DateFormatter+.swift Outdated Show resolved Hide resolved
Diary/Extension/DateFormatter+.swift Outdated Show resolved Hide resolved
Diary/View/DiaryCollectionViewListCell.swift Outdated Show resolved Hide resolved
Diary/View/DiaryCollectionViewListCell.swift Outdated Show resolved Hide resolved
@idinaloq idinaloq marked this pull request as draft August 30, 2023 05:33
@idinaloq
Copy link
Author

중복되는 코드의 경우 프로토콜로 정의한 뒤
프로토콜의 extenstion을 활용하는건 어떨까요?

라는 말씀을 듣고 protocolextension 둘다 사용해보았는데, @objc코드 때문에 둘 다 사용이 불가능 했습니다. 혹시 @objc코드를 procotolextension에서 사용할 수 있는 방법이 따로 있을까요?

추가로, 해당 방법이 잘 되지 않아서 KeyboardManager라는 클래스를 만들어서 사용하도록 했습니다. 이 방법은 어떻게 생각하시나요?

@idinaloq idinaloq marked this pull request as ready for review August 31, 2023 09:09
@havilog
Copy link

havilog commented Aug 31, 2023

  1. keyboardManager를 만들어서 사용하는 방법도 틀리다고 생각하지는 않습니다.
    공통 코드를 처리하는 방법으로는,

  2. 말씀드린 protocol을 활용하거나,

  3. base class를 두고 상속을 활용하거나,

  4. 구현하신대로 기능을 가진 객체로 빼서 의존성을 주입시키거나
    하는 방법이 있을 것 같구요,
    해당 방법들의 차이점과 장단점을 공부하고 고민하셔서 면접에서 잘 대답하는게 좋을것 같아요.

  5. 수정된 부분에 대해서 관련 커밋 7a4276b 이런식으로 남겨주시면 리뷰에서 컴(커뮤니케이션)이 더 쉬워질것 같아요~

  6. 요거는 해도 되는지 잘 모르겠는데, 혹시 리소스(작업 일정에 여유)가 남으신다면 다음 프로젝트(일기장 끝나고)에서 하게될 mvvm도 예습해서 조금 적용해보시는 것도 검토해보시면 좋을 것 같네요~

@idinaloq idinaloq marked this pull request as draft September 1, 2023 02:13
@idinaloq idinaloq marked this pull request as ready for review September 1, 2023 03:26
@havilog havilog merged commit 99765da into yagom-academy:ic_9_idinaloq1111 Sep 1, 2023
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.

3 participants