-
Notifications
You must be signed in to change notification settings - Fork 48
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
chore: use dimension for most layout distances #201
Conversation
Define some dimensions in dimes.xml and reuse them as often as possible to unify the layout and make changing dimensions easier. (Might be a future feature to use more or less spacing)
android:textSize="@dimen/font_size_home_date" | ||
android:layout_marginTop="@dimen/_94sdp" | ||
android:layout_marginTop="@dimen/_88sdp" |
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.
should this be like margin_top_huge
?
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.
This one is special, as the top date is measured from the top of the parent, although the clock is in between. So maybe margin_top_home_date
would be better?
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.
agreed, that would be better. I wasn't entirely sure of the rationale for the change regardless, so this is helpful context
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.
uh, @jkuester this was awaiting changes
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.
Whoops! 🤦 I misinterpreted your last comment without realizing @khwolf had offered an alternative in the previous comment....
I also just cut the beta release 😅, but if this is a necessary change we can get another PR up and merged before dropping 2.1.0
...
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.
not necessary, it was just to round out the change. doesn't need to hold up the release
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.
Would like to hear your opinion to my two comments.
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.
Thank you! This will greatly help keep things more consistent going forwards!
Define some dimensions in dimes.xml and reuse them as often as possible to unify the layout and make changing dimensions easier. (Might be a future feature to use more or less spacing)