-
Notifications
You must be signed in to change notification settings - Fork 806
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
fix: time interval unit UI #2548
base: development
Are you sure you want to change the base?
Conversation
🧙 Sourcery has finished reviewing your pull request! Tips
|
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.
Hey @Dishebh - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/11644641928/artifacts/2136653247 |
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.
@Dishebh Thank you very much for the PR and your work !
Here's a screenshot of the layout on my device:-
I've added a suggestion as to how we can make this layout more flexible, to support more screen sizes.
Let me know how this sounds to you:))
@@ -94,7 +94,7 @@ | |||
android:layout_width="@dimen/width_zero" | |||
android:layout_height="wrap_content" | |||
android:layout_gravity="center" | |||
android:layout_weight="@dimen/weight_one" /> | |||
android:layout_weight="@dimen/weight_three" /> |
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.
Perhaps, instead of updating the layout_weight
, we could migrate the parent LinearLayout
to a ConstraintLayout
, which would better support a large number of screen sizes.
ee977bb
to
34ba15b
Compare
Fixes #2188
Changes
Screenshots / Recordings
Checklist:
strings.xml
,dimens.xml
andcolors.xml
without hard coding any value.strings.xml
,dimens.xml
orcolors.xml
.