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

더보기 탭 닉네임 기능 #211

Merged
merged 12 commits into from
Sep 6, 2023
Merged

Conversation

eastshine2741
Copy link
Collaborator

닉네임 표시 및 수정 기능

@eastshine2741 eastshine2741 requested a review from a team as a code owner August 16, 2023 13:09
val apiOnProgress = LocalApiOnProgress.current
val apiOnError = LocalApiOnError.current
val scope = rememberCoroutineScope()
val userViewModel = hiltViewModel<UserViewModel>()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UserConfigPage와는 다른 viewModel이 생기지만, userDto가 스토리지 단에서 갱신되기 때문에 UserConfigPage에서도 바뀐 닉네임이 잘 반영된다
그래도 찝찝한데.. 전에 이야기한 뷰모델 관련 리팩토링 할 때 이런 코드들 싹 바꾸면 좋겠다고 생각했어

Copy link
Collaborator

Choose a reason for hiding this comment

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

일단 ㄱㄱ 복잡하지 않은 페이지에서는 새 route에 뷰모델 막 생성해도 큰 문제 없으니까

@@ -18,6 +18,7 @@ object NavigationDestination {
const val TeamInfo = "team_info"
const val TimeTableConfig = "timetable_config"
const val UserConfig = "user_config"
const val ChangeNickname = "change_nickname"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이것도 iOS랑 딥링크 스킴 맞춰야해? 모든 navigation destination을 다 맞추는거?

Copy link
Collaborator

Choose a reason for hiding this comment

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

저기 쓴거대로 intent에 extra로 snutt://change_nickname가 들어오면 Navigation 라이브러리에서 파싱해서 저 route로 보내줌

Copy link
Collaborator

Choose a reason for hiding this comment

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

근데 settings/change_nickname으로 하자는 신홍의 이야기가 있어서 얘기해봐야함

@JsonClass(generateAdapter = true)
data class NicknameDto(
@Json(name = "nickname") val nickname: String = "",
@Json(name = "tag") val tag: String = "",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UserDto의 nickNameDto 필드에 초기값 들어있어서 하위호환성은 지키는 것 같은데, NicknameDto 내부의 필드들에도 초기값을 넣어야 하나
그냥 일관성있게 항상 넣는게 좋아?

Copy link
Collaborator

Choose a reason for hiding this comment

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

nullable이 아니면 초기값은 반드시 있는 게 좋지

@eastshine2741 eastshine2741 marked this pull request as draft August 16, 2023 13:20
),
)
nicknameRequirementTexts.forEach {
BulletedParagraph(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bulletpoint가 있는 문단을 구현해야 하는데, 문단 끼리의 줄 간격과 문단 내에서의 줄 간격이 달라야 한다
annotatedString만 가지고 해보고 싶었는데 잘 안돼서.. 일단 각 문단을 별개의 composable로 만들어서 이어붙임

Copy link
Collaborator

Choose a reason for hiding this comment

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

annotatedString이 매우 쓰기 불편하긴 하지

@eastshine2741 eastshine2741 marked this pull request as ready for review August 20, 2023 15:56
@@ -29,7 +29,7 @@ interface UserRepository {

suspend fun fetchUserInfo()

suspend fun putUserInfo(email: String)
suspend fun patchUserInfo(nickname: String)
Copy link
Collaborator

Choose a reason for hiding this comment

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

patch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

restapi 메서드가 patch로 바뀌어서 레포지토리에서도 그대로 patch로 썼어
레포지토리의 메서드 이름 지을 때 api의 메서드 따라서 짓는 거 맞아?

Copy link
Collaborator

Choose a reason for hiding this comment

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

별 규칙은 없긴해

Copy link
Collaborator

@JuTaK97 JuTaK97 left a comment

Choose a reason for hiding this comment

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

LGTM

NicknameEditText(
value = nicknameField,
onValueChange = { nicknameField = it },
onDone = { handleChangeNickname() },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

닉네임 필드가 빈칸이거나 기존 닉네임과 같은 경우 탑바의 저장 버튼을 비활성화해둠
그런데 키보드의 완료 버튼을 눌러서 저장하는 건 가능한 상태...
막을 필요 있나?? 문제 없이 잘 작동하긴 함

Copy link
Collaborator

@JuTaK97 JuTaK97 Aug 28, 2023

Choose a reason for hiding this comment

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

키보드 완료 버튼도 막으면 좋을듯? 그냥 handleChangeNickname 살짝만 바꾸면 되잖아

Copy link
Collaborator

Choose a reason for hiding this comment

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

닉네임 필드가 빈칸이거나 기존 닉네임이랑 같을 때 KeyboardActions(onDone = null) 로 하면 깔끔할거같은데

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 오늘 내로 수정해둘게여

@JuTaK97 JuTaK97 force-pushed the eastshine2741/more-tab-nickname branch from 8a8fb91 to 6394cb6 Compare September 6, 2023 08:27
@JuTaK97 JuTaK97 enabled auto-merge (squash) September 6, 2023 08:27
@JuTaK97 JuTaK97 merged commit 2086997 into develop Sep 6, 2023
3 checks passed
@JuTaK97 JuTaK97 deleted the eastshine2741/more-tab-nickname branch September 6, 2023 08:36
@JuTaK97 JuTaK97 mentioned this pull request Sep 18, 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.

2 participants