[ExtendedFloatingActionButton] Added option to hide full button on collapse #883
Conversation
…llapse Until this commit, Extended FABs could only partially collapse and kept the icon visible. Now, Extended FABs have an option to fully collapse, similar to the collapse behavior of the FloatingActionButton
| */ | ||
| public void setHideBehavior(boolean isHide) { | ||
|
|
||
| ExtendedFloatingActionButtonBehavior<ExtendedFloatingActionButton> extendedBehavior = (ExtendedFloatingActionButtonBehavior<ExtendedFloatingActionButton>) behavior; |
There was a problem hiding this comment.
We should probably just change the type of the behavior to be ExtendedFloatingActionButtonBehavior. I don't think it would be anything else.
There was a problem hiding this comment.
Okay, I will add that in...
lib/java/com/google/android/material/floatingactionbutton/ExtendedFloatingActionButton.java
Show resolved
Hide resolved
| * | ||
| * This option is "false" and perform a shrink by default. | ||
| */ | ||
| public void setHideBehavior(boolean isHide) { |
There was a problem hiding this comment.
A client could call getBehavior() and then call these methods directly. Do you think there's value in adding this wrapper method? Or could we just add documentation?
There was a problem hiding this comment.
Actually... they can't. ExtendedFabBehavior is marked protected so the ExtendedFabBehavior methods cannot be called. This created a need for a client method to be exposed.
I would not recommend changing the class to public since it is just this one case that needs to be addressed. It would be unnecessary to expose an entire separate class to call... well, one method
There was a problem hiding this comment.
However, if you want that to happen I would be glad implementing that.
I am just a contributor and I want to make API design decisions that match your needs.
There was a problem hiding this comment.
getBehavior returns Behavior<ExtendedFloatingActionButton>, doesn't it? A client can't cast that to protected ExtendedFloatingActionButton.ExtendedFloatingActionButtonBehavior in order to get to the properties without extending from ExtendedFloatingActionButton or somthing. Or am I missing something?
There was a problem hiding this comment.
You are missing nothing @falsevapor. That is the exact conversation we are having!
There was a problem hiding this comment.
@ethanhsuhsu, sorry, I missed your replies here. Somehow github just showed here the message from @cketcham only... nevermind
There was a problem hiding this comment.
Yeah.. the fact that that class is protected is what I was missing. I guess it makes sense for it to be protected since it's very specific to Extended Fab..
| * | ||
| * This option is "false" and perform a shrink by default. | ||
| */ | ||
| public void setHideBehavior(boolean isHide) { |
There was a problem hiding this comment.
Because of the addition of this method, it looks like this api is a bit confusing. I think if autoShrink is true it takes precedence over autoHide we could make this into an IntDef or something where you can only set the collapse behavior to one of them. For example, you could call setCollapseBehavior(HIDE) to perform the same thing as setHideBehavior(true), or setCollapseBehavior(SHRINK) for setHideBehavior(false). Then you'd also have any easy way to disable any of this by calling setCollapseBehavior(NONE). What do you think about adding that method and deprecating the others?
There was a problem hiding this comment.
That it what I invisioned, for an enumaration. Originally I just wanted to keep my changes minimal was to maintain the current API.
Great, glad you are onboard with the changes. I will change the booleans to enums in the client method and throughout the behavior class
There was a problem hiding this comment.
I don't know how realistic this is but is there absolutely no way to combine both? Does it have to be one OR the other? Why not? I see a possible need to first autoshrink the button when collapsing the app bar to the regular/thin toolbar and then autohiding altogether when the toolbar is also collapsed. It seems to me there's a scenario when you might want both.
There was a problem hiding this comment.
@falsevapor You may be on to something, how about you post that in a separate GitHub issue? I will address that when this is complete 😉
With this PR, we have to address some challenges integrating the ExtFABBehavior class. This PR does not to be more complicated than it already is.
There was a problem hiding this comment.
@cketcham What do you think about this decision. Should we make the ExtFABBehavior class public, or keep it how it is and expose a client method?
There was a problem hiding this comment.
right, for now just to allow the access to one or the other is enough to get my original question answered/handled.
There was a problem hiding this comment.
Thank you. Just let me know when you are ready for me to test this. Seems like it's still in discussion.
There was a problem hiding this comment.
I think having the option to first auto shrink and then hide would be great! But it's also outside the scope of this PR. Maybe we could change the api that you've added to ExtendedFloatingActionButton? Instead of having the method take a boolean, you could have it take an intdef or enum for HIDE, SHRINK, and NONE.
In the future, this should allow us to add new enum values that let us create more complicated behavior such as the option to do both like you've described.
There was a problem hiding this comment.
Agreed. I opened a new ticket about allowing both.
Until this commit, Extended FABs could only partially collapse and kept the icon visible. Now, Extended FABs have an option to fully collapse, similar to the collapse behavior of the FloatingActionButton