-
Notifications
You must be signed in to change notification settings - Fork 75
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 3] hoon, karen #140
base: ic_9_karen
Are you sure you want to change the base?
Conversation
Diary/Network/CacheManager.swift
Outdated
class CacheManager { | ||
static let shared = NSCache<NSString, UIImage>() | ||
|
||
private init() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.인터페이스로 아무것도 지원하지 않나요 ? 그렇다면 매니저라는 이름이 맞나요 ?
2. 상속을 하나요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 싱글톤 객체를 사용하지 않고 캐시 작업을 수행하는
DiaryCell
에 객체를 생성하여 사용하기에는View
에 불필요한 객체가 들어가는 것 같아CacheStore
로 네이밍을 변경하였습니다. final
키워드를 사용하였습니다.
Diary/Network/Model/WeatherAPI.swift
Outdated
|
||
import Foundation | ||
|
||
protocol URLalbe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름이 조금 어색하네요 ... ㅎㅎ
NetWorkManager가 API 형태로 받을수있는 프로토콜인거죠 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
삭제 해주었습니다.
Diary/Network/Model/WeatherAPI.swift
Outdated
return nil | ||
} | ||
|
||
return "https://api.openweathermap.org/data/2.5/weather?lat=\(latitude)&lon=\(longitude)&appid=\(APIKey)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL구성요소로 나눠봐도 좋았을 것 같습니다 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URLComponents
를 통해 각각의 구성요소에 맞게 URL
을 생성하였습니다.😊
Diary/Network/Model/WeatherAPI.swift
Outdated
|
||
var url: String? { | ||
switch self { | ||
case .weatherData(latitude: let latitude, longitude: let longitude): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latitude: let latitude,
맨 앞에 latitude 가 꼭 필요한가요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전달인자 레이블인 latitude
, longitude
를 생략하였습니다.
Diary/Network/Model/WeatherAPI.swift
Outdated
guard let file = Bundle.main.path(forResource: "WeatherInfo", ofType: "plist"), | ||
let resource = NSDictionary(contentsOfFile: file), | ||
let key = resource["API_KEY"] as? String else { | ||
fatalError("⛔️ API KEY를 가져오는데 실패하였습니다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API key를 못가져오면 크래시가 나나요 .. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toast
메세지를 사용하여 사용자에게 안내하였습니다.🍞
label.setContentHuggingPriority(.defaultHigh, for: .horizontal) | ||
|
||
return label | ||
}() | ||
|
||
private let weatherIconImageView = { | ||
let imageView = UIImageView() | ||
imageView.contentMode = .scaleAspectFit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.scaleAspectFit 말고 나머지는 뭐가 있고 특징은 무엇인가요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scaleAspectFit
: 가로 세로 비율을 유지하면서 뷰의 사이즈에 맞게 이미지를 늘리는 옵션 (비율이 맞지 않을 경우 내부에 여백이 생김)scaleAspectFill
: 가로 세로 비율을 유지하면서 뷰의 사이즈에 맞게 이미지를 꽉 채우는 옵션 (이미지의 한 부분이 잘려 보일 수 있음)scaleToFill
: 전체 이미지가 다 나올 수 있도록 이미지를 꽉 채우는 옵션 (비율 무시)center
: 이미지를 이미지 뷰의 가운데에 배치 (이미지가 이미지 뷰보다 크면 일부 이미지가 잘릴 수 있음).top
,.bottom
,.left
,.right
: 가로 세로 비율을 유지하면서 이미지를 이미지 뷰의 위, 아래, 왼쪽 또는 오른쪽에 배치(이미지가 이미지 뷰보다 크면 일부 이미지가 잘릴 수 있음).topLeft
,.topRight
,.bottomLeft
,.bottomRight
: 가로 세로 비율을 유지하면서 이미지를 이미지 뷰의 네 모서리 중 하나에 배치(이미지가 이미지 뷰보다 크면 일부 이미지가 잘릴 수 있음)
@@ -57,10 +66,45 @@ final class DiaryCell: UITableViewCell { | |||
fatalError("init(coder:) has not been implemented") | |||
} | |||
|
|||
func configureCell(title: String?, date: String, preview: String?) { | |||
override func prepareForReuse() { | |||
weatherIconImageView.image = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- super콜이 필요없나요 ?
- 왜 image = nil 이 필요한가요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super
콜을 수정하였습니다.image
에nil
이 필요한 이유는cell
이 재사용 될 때 기존에 날씨 아이콘 이미지가 없던cell
에도 재사용되기 전의 이미지가 들어가는 문제점이 발생해서 초기화를 위해 할당해주었습니다.
Diary/View/DiaryCell.swift
Outdated
return | ||
} | ||
|
||
guard let cache = CacheManager.shared.object(forKey: NSString(string: icon)) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
캐시된게 있으면 그걸 쓰고 아니면 불러오라는 뜻 같은데, 흐름상이나 가독성 면에서 if let이 더 좋아보였을 것 같기도 하네요 !
상단에서 빠른 종료가 아니라 있는지 없는지 여부를 체크할떄는 if guard 를 잘 생각해서 쓰면 좋을 것 같습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let
으로 수정하였습니다.🤗
// Created by hoon, karen on 2023/09/14. | ||
// | ||
|
||
enum NetworkError: Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return | ||
} | ||
|
||
let weather = WeatherAPI.weatherData(latitude: latitude, longitude: longitude) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weatherData( 는 조금 어색한 것 같습니다.
무엇을 위한 API 인지가 들어가는 이름이 적합해보입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localWeather
라는 네이밍으로 수정했습니다.😊
Stpe03 README 수정
README : 블로그 아이콘 이미지 다시 올림
STEP 3
안녕하세요.
July!
(@July911)😊hoon
🐝,keren
🦦입니다.마지막
Step03 PR
보냅니다.벌써
July
께 리뷰 받을 수 있는 3주가 지나갔네요.🥲July
덕분에 많이 배우고 깨달은 것 같습니다. 꼼꼼하고 친절한 리뷰 감사했습니다.천천히 보시고 리뷰 남겨주시면 감사하겠습니다.
🤔 고민한 부분
1️⃣ 오토레이아웃
일기장을 새로 만든 후에 날씨 아이콘을 받아와서 등록됐을때는 온전하게 오토 레이아웃이 적용됐는데 일기장을 들어갔다 나오면 날씨 아이콘이 커져버리는 문제점이 발생했습니다.
dateLabel
의 높이에 맞추어서 날씨 아이콘의 크기가 변경되도록 제약조건을 잡아주고dateLabel
의setContentHuggingPriority
을required
로 가장 높게 잡아주어 해결했습니다.2️⃣
API KEY
숨기기API KEY
를 감추기 위해plist
파일을 생성하여 외부에 드러나지 않도록 숨겼습니다.API KEY
를 위해 생성한 파일을git
에 저장하고 이후git
에서 추적하지 않도록 설정하여 실제KEY
값을 사용하였습니다.🙇🏻♀️ 조언을 얻고 싶은 부분
1️⃣ 날씨 아이콘 오토레이아웃
setContentHuggingPriority
에priority
값을defaultHigh(750)
으로 줄 때와required(1000)
로 줄 때 두 값에 따른 동작 부분이 이해하기 어렵습니다. 750으로 값을 주었을 때 충돌할 만한 조건이 없다고 생각하는데Cell
이 다시 그려질 때 주어진 제약조건과는 다르게 날씨 아이콘이 커지는 문제가 발생하였습니다.defaultHigh
보다 큰 값인 751 이상일 때는 문제점이 발생하지 않는데 어떤 부분에서 충돌이 발생했을지 궁금합니다.2️⃣ 시뮬레이터 CoreLocation 사용
Feature > Location > Custom Location
을 통해서 직접 위치 좌표를 기입하는 방법이 아닌 GPS 기반으로 실제 테스트하고 있는 장소의 위치 정보를 받을 수 있는 방법이 있을지 궁금합니다. 시뮬레이터에 기본으로 설치되어 있는 지도 앱을 테스트한 경우에도 현재 위치가 아닌 기본 값으로 설정되어 있는 위치 좌표 값을 받아오고 있었습니다.