UI updates for ButtonWidget #4594
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
|
Thats looks nice! I see some different top and leading padding, can you check that? For reference, this is the iOS view https://github.com/home-assistant/iOS/blob/master/Sources/Extensions/Widgets/Common/WidgetBasicView.swift |
Quite big, let's not increase the scope of the PR here. (And personally, I think a combined widget with entity state, which has an easy toggle/run instead of requiring you to configure each action, would make more sense for large amounts of items.) |
Hi @bgoncal, the different padding is totally a personal preference given the default appearance of the widget is more rectangular shaped I tried leaving less padding horizontally than vertically to allow a little bit more space for the label. I can change it to an uniform padding (given the widget can be also resized). Also as @jpelgrom mentioned, I assume a multi-item widget would be a completely different kind of widget, with its own configuration so possibly a bit too outside the scope of the PR. |
|
Agree on having a separate PR for multiple items widget ✅ |
app/src/main/java/io/homeassistant/companion/android/widgets/button/ButtonWidget.kt
Outdated
Show resolved
Hide resolved
...main/java/io/homeassistant/companion/android/widgets/button/ButtonWidgetConfigureActivity.kt
Outdated
Show resolved
Hide resolved
| android:id="@+id/widgetImageButtonLayout" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:paddingVertical="4dp" | ||
| android:layout_height="wrap_content" |
There was a problem hiding this comment.
The size change will create visual problems with longer labels and/or smaller widget sizes:
(default size, and this label isn't even that long)
There was a problem hiding this comment.
This has not been resolved. It may be less obvious now, but text is cut off (note the bottom of the g/p), and especially for users with larger font sizes configured this will happen quite easily.
There was a problem hiding this comment.
Yeah I figured the issue is now caused by the text's size rather than its layout overlapping the above icon. I can reproduce it only if I manually set my font size in the phone's setting to the maximum, otherwise it looks good to me. Is that the case here as well?
There was a problem hiding this comment.
No, this is on the default text and display size for me (Pixel 7a; text size 2/7, display size 2/5).
There was a problem hiding this comment.
Please see this review comment.
|
Added a commit that fixes most of @jpelgrom's remarks.
I believe they're equivalent? I'm not sure if there's any specific reason to use either. |
| @@ -228,6 +233,10 @@ class ButtonWidget : AppWidgetProvider() { | |||
| // Render the icon into the Button's ImageView | |||
| setImageViewBitmap(R.id.widgetImageButton, icon.toBitmap(width, height)) | |||
|
|
|||
| widget?.iconColor?.let { | |||
| setCustomColorToIcon(it, this) | |||
There was a problem hiding this comment.
This will overwrite the color set to the icon in line 204/206, which is fine if you have set a custom color but when using the transparent theme it might create a mismatch (black icon on a colored circle with white text, while the option says it'll color text + icon, and it just looks odd).
Summary
Implementation of a fresher look for Button widget as suggested in #4549, adding a parameter to make the icon's color customizable.
Screenshots
Config:
Day:
Night:
Dynamic:
Transparent:
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#1097
Any other notes