Issue #45: support for preventing spinner from hiding on Android back #46
Conversation
| | ||
| static defaultProps = { | ||
| visible: false, | ||
| cancelable: true, |
There was a problem hiding this comment.
@niftylettuce WDYT? IMO, this should be false by default to not introduce a braking change.
There was a problem hiding this comment.
@SandroMachado I agree, should be false and we should update Readme too if it isn't already
There was a problem hiding this comment.
@niftylettuce @SandroMachado The native Android Dialog has a setCancelable method. Since the new prop that I've added here only applies for Android, I thought it makes sense to name it after the native one. What do you think?
There was a problem hiding this comment.
LGTM, probably the only thing I would change it is from true to falsein the default value of the prop. To be disable by default.
There was a problem hiding this comment.
@SandroMachado @niftylettuce There are 2 main reasons I would vote against setting the default value to false:
- By setting the default value to
falseyou will introduce a change since currently the spinner is cancelable. Therefore users who will get the update and want to keep the same behavior as the current version would have to explicitly setcancelable:true. - The native Android Dialog is by default cancelable.
If you still insist, I can change it though.
There was a problem hiding this comment.
@gsavvid you are right. Can you please only update the README.md with this new prop?
There was a problem hiding this comment.
@SandroMachado I took the liberty to re-organize a bit the props part in the Readme.md file by showing them all in a table, ordered alphabetically.
One thing I also updated in the documentation is that -according to the source code- the size prop supports normal but only small and large was documented.
If you don't want these changes, I can revert the commit and simply add a new bulletin with the cancelable prop.
| @SandroMachado did you want to version bump and release new version to NPM? I can if you need, thanks - just let me know. 🎉 |
| @niftylettuce if you have time, please do it. I can only do the release later today. |
| released 0.4.3 |
| I unpublished 0.4.3 - I have published 0.4.4 with a default of |
3 participants
On Android there's the need of preventing spinner from hiding when pressing the hardware back button. This can be resolved by setting the new prop
cancelabletofalse-by default it'strue.This has no effect on iOS as the Modal's
onRequestCloseprop is Android-specific.