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

Feature/UI #18

Merged
merged 15 commits into from
Dec 16, 2024
Merged

Feature/UI #18

merged 15 commits into from
Dec 16, 2024

Conversation

Guri999
Copy link
Collaborator

@Guri999 Guri999 commented Nov 29, 2024

No description provided.

@Guri999 Guri999 self-assigned this Nov 29, 2024
@Guri999 Guri999 added the ✨ feat 기능 개발 label Nov 29, 2024
) {
LazyVerticalGrid(
modifier = Modifier
.padding(

Choose a reason for hiding this comment

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

contentPadding을 사용하지 않고 Modifier의 padding을 사용한 이유는 무엇인가요?

) {
Icon(
modifier = Modifier.size(32.dp)
.clearAndSetSemantics { },

Choose a reason for hiding this comment

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

clearAndSetSemantics는 어떤 의도로 추가했나요?

.clearAndSetSemantics { },
imageVector = SeeDocsIcon.PDF,
contentDescription = null,
tint = Color.Unspecified

Choose a reason for hiding this comment

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

Color.Unspecified 로 설정하신 이유가 궁금합니다.


import androidx.lifecycle.ViewModel
import dagger.hilt.android.lifecycle.HiltViewModel
import javax.inject.Inject

Choose a reason for hiding this comment

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

의미없는 코드는 커밋에 포함되지 않도록 하시는게 좋을 것 같아요.
이를 방지하기 위해선 개발 계획을 잘 세우시는게 중요합니다.

@@ -13,6 +18,21 @@ internal fun MainNavHost(
navController = navigator.navController,
startDestination = navigator.startDestination,
) {
exploreNavGraph(
padding = padding,
navigateToPdf = { navigator.navigate(Route.Pdf){null} }

Choose a reason for hiding this comment

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

navigator.navigate(Route.Pdf){null}
코드 정렬 확인 부탁드립니다.

.fillMaxSize()
.background(Theme.colors.bg)
) {

Choose a reason for hiding this comment

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

구현이 필요한 코드는 TODO 코멘트를 남겨놓는 것도 괜찮은 방법입니다.

Spacer(Modifier.height(32.dp))

Text(
text = "북마크",

Choose a reason for hiding this comment

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

앱 내에 필요한 텍스트는 하드코딩보다 strings.xml 사용을 권장드립니다.

Copy link

@f-lab-dean f-lab-dean left a comment

Choose a reason for hiding this comment

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

LGTM, 머지하셔도 좋습니다.
다음 PR부터는 PR 코멘트에 대한 답글을 달아주세요.

@Guri999 Guri999 merged commit f0b0ee8 into main Dec 16, 2024
2 checks passed
@Guri999 Guri999 deleted the feature/ui branch December 16, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feat 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants