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

Button Widget: Uniform Spacing #3099

Merged
merged 3 commits into from Nov 29, 2022

Conversation

tanishqmanuja
Copy link
Contributor

Summary

Button widget had asymmetrical top and bottom spacing. This PR tries to fix that.
Related issue: #3093

Screenshots

Portrait:
image

Landscape:
image

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

  • The icon named old is from stock Home Assistant App kept for comparison with the newer ones.
  • The only small downside of this approach is that on multiline text the icon becomes slightly smaller as compared to stock multi line icon (BTW stock multiline widget is not present in the screenshots above)

In order to fully solve this only downside with even better but complex approach is to use multiple layouts:
https://developer.android.com/develop/ui/views/appwidgets/layouts#provide-responsive-layouts

@jpelgrom
Copy link
Member

This still isn't quite equal spacing, unless I'm missing something. The root view widgetLayout has 4dp padding all around, and the label view widgetLabelLayout has 2dp padding all around, so in total there is now 4dp padding at the top but 6dp padding at the bottom.

There is also an additional 4dp vertical padding for widgetImageButtonLayout - why is that? It contributes to the smaller icon you're describing.

The entity state widget uses very similar spacing for it's elements, if the button widget is updated I feel like that one should be too in this PR to keep their layouts aligned.

In combination with your previous PR, should this close the issue?

@tanishqmanuja
Copy link
Contributor Author

tanishqmanuja commented Nov 25, 2022

Maybe not perfectly symmetrical (with difference of 2dp that too only on some devices).

Top Spacing -> 4dp from Widget Layout + 4dp from Vertical padding of Linear Layout
Bottom Spacing -> 4dp from Widget Layout + 4dp from Vertical padding of Linear Layout + 2dp from margin of Label Layout (to protect text character like 'j', 'g' which in my experience leaks on user fonts in Custom ROMs)

TLDR:

  • So the original 4dp from Widget Layout was already there before my PR, so I didn't touch it. I added extra 4dp to make total 8dp which is the Widget Corner Radius.
  • In combination with your previous PR, should this close the issue? Yess :)

PS:
2dp not adding to bottom spacing or doesn't seem to be noticeable when enough space is provided according to layout bounds.
image

@jpelgrom jpelgrom linked an issue Nov 25, 2022 that may be closed by this pull request
@tanishqmanuja
Copy link
Contributor Author

tanishqmanuja commented Nov 25, 2022

@jpelgrom, Regarding the entity state widget what should I do ?
BTW in my testing I found stock widget is already broken in multiline scenarios for landscape mode.

Portrait:
image

Landscape:
image

@tanishqmanuja
Copy link
Contributor Author

Commit f3a964e,
Mirror the changes from button widget to entity state widget.

Screenshot:
image

@jpelgrom
Copy link
Member

I added extra 4dp to make total 8dp which is the Widget Corner Radius.

The widget corner radius, especially when using the Dynamic color theme like you do, will depend on the widget theme, Android version, launcher, device and/or manufacturer. If you want to use the widget corner radius (which can be >8dp), you can use ?dimenWidgetCornerRadius.

Regarding the entity state widget what should I do ?

I think changes to mirror the button widget are good 👍

I found stock widget is already broken in multiline scenarios for landscape mode

There is a big chance it will be too small when using a modern phone, but for less tall devices / tablets it is less of a problem. Right now the text size is configurable by users so it's not easily solved without adding more options, changing it to a less exact option like 'small/medium/large' or adding a completely different layout, which is IMO not in the scope for this PR.

(Adding more padding like you've done does increase the chances of there not being enough space for the text.)

@tanishqmanuja
Copy link
Contributor Author

So is the PR complete or something more should be done?

@jpelgrom
Copy link
Member

Not uniform but close enough when there is a single line of text 😉

@JBassett JBassett merged commit 9bf5299 into home-assistant:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird Spacing in Button Widget
3 participants