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

Fix Feedback FAB margins #878

Merged
merged 1 commit into from
Apr 24, 2018
Merged

Fix Feedback FAB margins #878

merged 1 commit into from
Apr 24, 2018

Conversation

Guardiola31337
Copy link
Contributor

Noting that this only happened in devices with low resolution (e.g. Motorola TC55) so friendly reminder here that we should test on those devices as well.

feedbackbuttonproperlyalignedlowresdevice

In any case, the distance between sound and feedback buttons is not optimal for that resolution ☝️ (too long IMO), should we consider adding layout support for lower resolutions?

cc @devotaaabel @danesfeder

@danesfeder
Copy link
Contributor

should we consider adding layout support for lower resolutions

Can we add a layout with lower res qualifier to address this. Dropping the margins to 8dp will result in the buttons being too close on high-res devices like the Pixel 2

@Guardiola31337
Copy link
Contributor Author

@danesfeder

Can we add a layout with lower res qualifier to address this.

Added dimens.xml "default" and mdpi values support in 7326804

BTW we should revisit all hardcoded "dimens" values and 👀 if we want to support any other configurations (screen densities, sizes, etc.).

@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>
<dimen name="fab_margin_bottom">8dp</dimen>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we adjust this to 16dp? On higher res devices, the buttons are too close imo, may be more easy to click the wrong one:

device-2018-04-24-083032

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, that's not necessary 👀 fab_margin_bottom which doesn't affect the distance between sound and feedback buttons (it's the same exact value as it was).

@Guardiola31337
Copy link
Contributor Author

Added dimens.xml "default" and mdpi, hdpi, xhdpi, xxhdpi and xxxhdpi values support in 5134763

BTW we should revisit all hardcoded "dimens" values and 👀 if we want to support any other configurations (screen densities, sizes, etc.).

And also adjust hdpi, xhdpi, xxhdpi and xxxhdpi values so look ✨ for each screen density (now all of them define the same values as the "default" ones).

@danesfeder could you confirm that #878 (comment) is working as expected now?

Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, this is working correctly now. Thanks @Guardiola31337 :shipit:

@Guardiola31337 Guardiola31337 merged commit 91836f9 into master Apr 24, 2018
@Guardiola31337 Guardiola31337 deleted the pg-859-feedback-button branch April 24, 2018 14:50
@Danny-James
Copy link

@danesfeder should this now be available in the 13.0-SNAPSHOT ?

@danesfeder
Copy link
Contributor

@Danny-James yes 👍

@Danny-James
Copy link

@danesfeder Great Stuff, I've tested this and it's working.

@Guardiola31337
Copy link
Contributor Author

@Danny-James Great to hear, thanks for the update.

@danesfeder danesfeder mentioned this pull request May 3, 2018
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants