-
Notifications
You must be signed in to change notification settings - Fork 0
[Feat] #553 - 피드 목록 및 마이페이지 피드 이미지 첨부 UI 구현 #557
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
Conversation
Naknakk
left a comment
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.
고생했습니다! Entity-DTO가 분리되어서 추후 서버 작업이 들어와도 Repository 단에서 서버 의존성이 해결되니 빠르게 작업 가능하겠네용!
이미지가 들어가니 이제 앱이 더 멋있어지는군용..
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.
W1 to @ena-isme
이 부분 코드는 Entity 정리가 더 필요하다는 느낌이 드네용 . .
https://github.com/Team-WSS/WSS-iOS/pull/496/files/763124a055730c9857192de4ad1752729a16b536#diff-20960516213545e5b8765a72cbb7a46c8694dd91168b1bb4f017019d14efefbc
이 PR에서 더 자세히 봤어야 했는데 ㅜ 지금 다시 보니 연결된 웹소설 자체를 하나의 구조체로 뺀다음에, 연결된 소설이 있는지 여부를 판단하는 Bool 값이 있는게 더 좋을 것 같아요. 지금처럼 -1로 다 넘기면 의미를 알 수 없는 코드가 너무 늘어나는 느낌입니다. 추가로 Entity의 List 네이밍도 배열로 쓰이는 타입에 반대로 들어가있네요....
뒤늦은 리뷰지만 이 PR 이후 꼭 수정되면 좋을 것 같아요
| $0.height.greaterThanOrEqualTo(25) | ||
|
|
||
| imageCountLabel.snp.makeConstraints { | ||
| $0.edges.equalToSuperview().inset(imageCountBackgroundView.layoutMargins) |
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.
W2
신기하네요...??? 이런게 되는군요!! 다만 가독성 측면에서 레이아웃 관련 코드가 setLayout에 모여있는게 저는 좀 더 편한 것 같아서 이렇게 작성하는게 더 장점이 있는지는 아직 고민입니다 .. !
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.
그쵸 아무래도 .. 저도 그 점이 맘에 안들었긴 했습니다 ㅠㅠ
| imageCountBackgroundView.layoutIfNeeded() | ||
| imageCountBackgroundView.layer.cornerRadius = imageCountBackgroundView.frame.height / 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.
W1
setLayout() 메서드는 뷰의 init 시기에 실행되기 때문에, Label의 숫자 값이 바뀌는 것에 따라 BackgroundView의 CornerRadius를 업데이트 하려는 의도대로 되지 않을 것 같다고 생각이 듭니다.. Label의 내용에 따라 cornerRadius를 바꿔주려면, bindData 쪽에서 레이아웃 처리를 다시 한 번 해줘야 할 것 같아요.
또는, 숫자가 바뀜에 따라 Label Height가 극적으로 바뀌지 않을 것이라 예상하는데, imageCountBackgroundView의 height를 25로 그냥 고정해두고 맞춰서 cornerRadius를 주는 것도 나쁘지 않을 것 같다고 생각합니다. 피드는 무한스크롤이 필요할 정도로 양이 많은 만큼 데이터 할당할 때마다 다시 레이아웃을 잡기 보다는.. 고정해두는게 성능적으로도 더 좋을 것 같구요.
imageCountLabel는 이제 centerY를 superview와 동일하게 해두면 디자인 상 크게 거슬리지 않을 것 같습니다!
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.
오호 이해했습니다..
무조건 텍스트의 사이즈에 기반해서 뷰 크기를 잡아야한다는 생각에 layoutIfNeeded()를 호출했던 것 같아요.
해당 뷰가 그렇게 큰 중요성을 가지는 뷰도 아닌 것 같고 이 뷰를 위해 성능적 부담을 갖게 하는 건 과한 것 같네요!
말씀하신 것처럼 height = 25로 고정하고 좌우 여백을 할당하는 방향으로 변경해보겠습니다.
항상 좋은 리뷰 감사합니다 낙낙~~👍
| func putFeed(feedId: Int, relevantCategories: [String], feedContent: String, novelId: Int?, isSpoiler: Bool, isPublic: Bool) -> Observable<Void> | ||
| } | ||
|
|
||
| struct TestFeedRepository: FeedRepository { |
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.
W3
테스트 레포 활용 좋네요~! 근데 데이터가 너무 길게 들어가니, Entity 구조체에 DummyData Static 변수를 추가해서 사용하는 것도 좋을 것 같습니다! 데이터 셋을 여러개 만들어서 바꿔보기도 편하니까용!
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.
오 이것도 좋은 방법인 것 같네요!
테스트 레포가 너무 길어져서 거슬리긴 했는데! 반영해볼게요 👍
ena-isme
left a comment
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.
현재 스포일러가 아닌 모든 피드에서 같은 이미지가 표시되고 있는데, 의도한걸까요 ?
테스트용으로 이미지 넣어둔것이구요! 마이페이지는 레포지토리 만들기엔 내용이 넘 많아서 못만들었어요 ㅠ 어프룹 되면 이미지 없애놓을 예정입니닷 |
넵 마이페이지 내용은 PR에서 확인했습니다. |
이미지 없는 상황일때엔 피드 목록에서 확인하실 수 있습니다! @ena-isme |
|
@Guryss |
ena-isme
left a comment
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.
수고하셨어요!
| //TODO: DTO 수정 | ||
| thumbnailImage: "", | ||
| hasImage: true, | ||
| imageCount: 20 | ||
| ) |
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.
W3
아하 이부분때문에 그랬던 거군요! 코드 먼저 봤어야 했네요
분기처리 하고 다음 PR 때 사진유무에 따른 UI 확인 할게요!
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.
고생하셨습니당~!~! 깔끔하네요!!
아주 사소하지만 $0.height.equalTo(25) 에서 만들어둔 변수 backgroundHeight 를 넣어주면 더 좋을 것 같아용!
⭐️Issue
🌟Motivation
🌟Key Changes
MypageRepository는 안에 꽤나 많은 함수들이 있어서 일일히 추가하기 어려울 것 같아 일단FeedRepository만 반영했습니다.FeedViewController 에서 레포지토리만 바꿔 넣어주시면 됩니다.
내부 마진을 설정하여
imageCountLabel이 해당 마진만큼 여백을 갖도록 했습니다.숫자값에 따라 칩의 width, height이 변해서 아래처럼 height 레이아웃을 지정한 뒤 뷰를 강제 업데이트 한 뒤, cornerRadius를 height의 절반만큼 가져갈 수 있도록 설정했습니다. 해당 코드에서 개선할 부분이 있다면 피드백 부탁드립니다!
🌟Simulation
🌟To Reviewer
테스트 레포지토리엔 데이터를 4개 넣어놨는데 무한 스크롤때문인지? 데이터가 계속 늘어나네요..?
이 부분은 다시 찾아봐야 할 것 같습니닷
🌟Reference