-
Notifications
You must be signed in to change notification settings - Fork 1
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
DEP-148 ui: history UI 구현 #46
Conversation
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.
수고하셨습니다!!
<androidx.cardview.widget.CardView | ||
android:id="@+id/cv_tooltip" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginEnd="14dp" | ||
android:layout_marginTop="12dp" | ||
android:elevation="8dp" | ||
android:visibility="gone" | ||
app:cardCornerRadius="10dp" | ||
app:layout_constraintEnd_toEndOf="parent" | ||
app:layout_constraintTop_toBottomOf="@+id/iv_tooltip"> |
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.
white에 radius가 10인게 여러개 있는데 따로 drawable을 만들어서 textView의 background로 적용하시는건 어떠실까요?!
그럼 재활용성도 높아질 것 같고 view의 depth도 낮아질 것 같아서요!
(그냥 제안입니닷!!)
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.
좋은 제안인 것 같습니다 👍적용해볼게요!
<androidx.constraintlayout.widget.Group | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:visibility="visible" | ||
app:constraint_referenced_ids="cl_this_month_clap, cl_this_month_achieve_days, cl_most_achieve, tv_ongoing_habit, rv_habit" /> | ||
|
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.
신기하네요 !! 덕분에 좋은것? 알아갑니다!! 적용해서 제 코드도 수정해봐야겠어요👍
<shape xmlns:android="http://schemas.android.com/apk/res/android" | ||
android:shape="rectangle"> | ||
|
||
<solid android:color="@color/main" /> |
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.
color main이 변경되고 gray800으로 바껴서 (근데 아직 몇몇 디자인에는 main이 남아있더라구요) gray800으로 바꿔주시면 감사하겠습니다!
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.
후,,, 뭔가가,,, 색상이 또 바뀌었나보군여,, 알려주셔서 감사합니다 🥲
@@ -29,6 +29,10 @@ fun View.invisible() { | |||
this.isInvisible = true | |||
} | |||
|
|||
fun View.switchVisible() { |
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.
toggle 로 동작하게되면 이전상태에 의존하게되어서, 결과를 예측하기 어려울거같은데요.
멱등하게 동작하는건 어떻게 생각하시나요?
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.
첫 줄 보고 흠 그런가 했는데
멱등을 말씀하시니 반박을 못하겠네요. 🤐
수정했습니다...ㅎ..
<style name="Typography.Button2"> | ||
<item name="android:textSize" tools:ignore="SpUsage">15dp</item> | ||
<item name="fontFamily">@font/suit_bold</item> | ||
<item name="fontFamily">@font/suit_semi_bold</item> | ||
</style> | ||
|
||
<!-- Button3 --> |
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.
주석도 같이 Button2 로 바뀌어야할거같습니다
@@ -74,7 +74,7 @@ | |||
android:layout_height="wrap_content" | |||
android:layout_marginStart="8dp" | |||
android:gravity="center" | |||
android:textAppearance="@style/Typography.Body2.SemiBold" | |||
android:textAppearance="@style/Typography.Body2" | |||
android:text="습관 만들기" |
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.
여기 text 부분은 string.xml 에서 가져오지 않아도될까요?
혹시 의도하신거라면 string.xml 에서 가져오거나 가져오지 않는 기준이 있는지 궁금합니다
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.
의도한 건 아니고 저번에 strings.xml로 옮기는 작업을 할때
history 브랜치라 history꺼만 하고
이건 home꺼라서 남아있었습니다! 수정하겠습니다
@@ -41,7 +41,7 @@ | |||
android:layout_height="wrap_content" | |||
android:layout_marginStart="8dp" | |||
android:text="습관 만들기" |
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.
이 text 도 @string/create_habit
값으로 대체가능할거같습니다.
…-android into feature/DEP-148_history_ui
Kudos, SonarCloud Quality Gate passed! |
💁♂️ 변경 내용
AS-IS
TO-BE
📢 전달사항
이제 기능 들어가야 하기 때문에 임시 텍스트 다 빼버렸습니다. (뷰는 다 그려진 상태입니다)
tools로 적용해두었기 때문에 안드로이드 스튜디오에서는 비어있는 칸들도 채워져서 보실 수 있습니다.
아 그리고 이번에 group이라는 걸 처음 알게 됐는데
이미 아시겠지만...? 공유해보겠습니다.
지금 화면처럼 한 페이지에서 상태에 따라 다른 화면을 보여줘야 할 때 좋더라구요.
이렇게 하면 참고하고 있는 아이디의 뷰의 visible을 한 번에 관리할 수 있습니다.
이미 알고 계셨다면 죄송... 총총총