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

[feat] 피드 상세 페이지 구현 #83

Merged
merged 12 commits into from
Aug 12, 2023
Merged

[feat] 피드 상세 페이지 구현 #83

merged 12 commits into from
Aug 12, 2023

Conversation

ows3090
Copy link
Contributor

@ows3090 ows3090 commented Aug 9, 2023

Related issue

Work Description

  • FeedDetailScreen 구현
  • Store 상세 API 호출하여 피드 이미지 Pager 구현

Screenshot (선택)

Copy link
Contributor

@lee989898 lee989898 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

Comment on lines 13 to 14
androidxPaging = "3.1.1"
androidxPagingCompose = "1.0.0-alpha18"
Copy link
Contributor

Choose a reason for hiding this comment

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

image

3.2.0 version대신 3.1.1 version쓴 이유가 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lee989898 아 한글 문서로 봤을 떄는 3.1.1하고 1.0.0-alpha18로 나오는데 영어 문서로 보면 저렇게 나오는구나
수정할게!

modifier = Modifier
.size(116.dp)
.padding(top = 3.dp)

Choose a reason for hiding this comment

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

이거 LazyVerticalGrid 로 사용하면 VerticalArrangement 랑 HorizontalArrangement 있지 않나..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

우리 디자인이 가운데 있는게 아니고 양쪽 끝은 패딩이 없고 가운데만 있는 구조라서 애매하더라고..

Choose a reason for hiding this comment

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

그러면 LazyVerticalGrid 에서 ContentPadding 에가다가 Horizontal을 두고 HorizontalArrangement 를 두는게 가독성 면에서 더 좋지 않을까 라는 생각이 들어!

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

한번 봐볼게~~!! 떙큐땡큐

Copy link
Contributor Author

Choose a reason for hiding this comment

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

완전 꿀팁이네.. 완전 좋음.. 땡큐

onNavigateHomeDetail: (Int) -> Unit,
onClose: () -> Unit
) {
LaunchedEffect(storeId) {

Choose a reason for hiding this comment

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

뭔가 LaunchedEffect 를 화면 받아올 때마다 하고 있는데, 이걸 @AssistedInject 를 사용해서 하는거랑 어떤게 더 좋을지 생각해봐도 좋을 것 같다..! 둘다 큰 차이는 없을 수도 있는데, 나도 확실하지 않아서!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junwooYeom 이부분은 준경이랑 이야기 했는데 잘 모르겠어서 추후 스터디 시간에 설명듣고 반영해보는 걸로 할게 ㅠㅠ

}
}

@OptIn(ExperimentalFoundationApi::class)

Choose a reason for hiding this comment

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

이제는 OptIn 같은거 Compiler 에서 opt=in 거는 방법이 있어서 그건 한번 찾아봐도 좋을듯! 너무 많아지면 좀 코드가 약간 불안전한가..? 란 생각이 드는 경우도 난 있었던 것 같아서..!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junwooYeom 이부분도 동일하게 잘 모르는 내용이라 다음에 한번 물어보고 한꺼번에 반영할게!!

Box(
modifier = Modifier
.align(Alignment.TopEnd)
.padding(top = 20.dp, end = 16.dp)

Choose a reason for hiding this comment

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

Box 안에 Padding 을 두개 넣은 이유가 어떻게 돼??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

강남구, 용산구 과 같은 지역 정보가 오른쪽 상단에 존재하는데 상단에서 20dp 오른족으로 16dp만큼 패딩이 있어서 그렇게 했어

Choose a reason for hiding this comment

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

그러면 밑에 vertical 이랑 horizontal 을 추가로 넣은건 background 관련 해서 padding 을 추가로 넣은거야??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

응응 맞아맞아!! 텍스트가 있고 그 주변이 일정 간격 떨어진채로 배경이 있는 구조라서!

.padding(horizontal = 16.dp)
.fillMaxWidth()
) {
Box(

Choose a reason for hiding this comment

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

Box 를 안쓰고, Text 만 쓸 수 있는 방법이 있을 것 같은데...! contentAlignment 를 textAlignment 로 쓰고, Modifier 를 Text Modifier 로 써볼 수 있지 않을까? ( 이건 나도 시도해봐야 알 것 같은데 Box 가 한번 더 들어가면 레이아웃을 하나 더 그리는 게 되다보니까 Text 만 쓰는게 나을 수도 있을 것 같아서! )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junwooYeom 높이가 글자 크기보다 큰 상태에서 가운데로 오게끔 하기 위해서는 Box를 써야하는 것 같더라고..
Text로 해보려고 했는데 뭔가 잘 안되었던 것 같아 ㅠㅠ

@ows3090
Copy link
Contributor Author

ows3090 commented Aug 12, 2023

@junwooYeom @lee989898 빠른 출시를 위해 일단 머지하고 리팩토링 할 것들은 스터디시간에 조금 더 물어보고 이슈 생성할게요~~!!

@ows3090 ows3090 merged commit 624b406 into develop Aug 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] 피드 상세 페이지 구현
3 participants