Added SetFabElevation - Githubissues

leinardi / FloatingActionButtonSpeedDial

A Floating Action Button Speed Dial implementation for Android that follows the Material Design specification (https://material.io/components/buttons-floating-action-button#types-of-transitions)

Apache License 2.0

1.47k stars 142 forks source link

Added SetFabElevation #172

Closed PhantomLord72 closed 3 years ago

PhantomLord72 commented 3 years ago

In Builder and Parcelable too.

First time contributor checklist

[x] I have read how to contribute to this project
[x] I have read the code of conduct to this project

Contributor checklist

[x] I am using the provided codeStyleConfig.xml
[x] I have tested my contribution on these devices: Samsung A20e Android 10 Virtual device, Android 27 (8.1)
[x] My contribution is fully baked and ready to be merged as is
[x] I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

These 2 patches add setFabElevation on the custom view FabWithLabelView. The class SpeedDialActionItem has this new method and the Builder too. The default value for the elevation is 0.

leinardi commented 3 years ago

Hi @PhantomLord72, thank you for your PR.

A couple of things before I check it:

[ ] could you please merge #172 and #173 in a single PR? The easiest solution would be for you to fork the project, push both changes on your forked repository and then make a PR to this
[ ] could you please target the master branch instead of the release branch?
[ ] could you please double check that you are using the current elevation value instead of setting it to 0 by default? I can't merge your changes if the default elevation of the FABs changes from the current due to your changes

PhantomLord72 commented 3 years ago

N.1 and 2 are ok. N.3: the default elevation set in master branch is 1, you can see in sd_fab_with_label_view.xml, according to this I have set the default to 1 too (not 0).

leinardi commented 3 years ago

N.3: the default elevation set in master branch is 1, you can see in sd_fab_with_label_view.xml, according to this I have set the default to 1 too (not 0).

that is actually the elevation of the CardView, I would expect the FAB to have a different elevation by default (not sure, but I would guess at least 4dp). So this can be a problem: CardView and FAB currently I believe they have different elevation values.

Also, keep in mind that the the current value of the CardView elevation is 1dp, not 1px so you'll need to convert the dp value to px...

PhantomLord72 commented 3 years ago

So, there are 3 elevation to consider: -of the main fab, set fixed in SpeedDialView code, there are 2 elevation, on open and closed (why 2 different?). -of the card -of the (mini)FAB: in setLabelBackgroundColor there is something wrong; mLabelCardViewElevation is in an "if" before an assignment. Maybe, for consistency, it is better to have an equal elevation across all the three? And to have no distinct elevation for open fab and closed fab?

leinardi commented 3 years ago

the 2 different elevations are needed to ensure that the overlay layout doesn't go on top of the FABs. Frankly I'm not sure if makes sense to allow the user to change the elevation. What is the use case?

PhantomLord72 commented 3 years ago

Only aesthetics. Then primary :) Feel free to reject my pr