Skip to content

Issue #45: support for preventing spinner from hiding on Android back #46

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 4, 2017
Merged

Issue #45: support for preventing spinner from hiding on Android back #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d930938
Issue #45: support for preventing spinner from hiding on Android back
gsavvid Mar 23, 2017
65ce8b9
Updated Readme.md
gsavvid Apr 4, 2017
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions Readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,17 @@ npm install --save react-native-loading-spinner-overlay@0.1.x

This usage shows the default styles and properties.

* You can pass a String `size` prop that can either be `"large"` or `"small"` (no other cross-platform sizes are supported right now, and by default it is `"large"`)
* You can pass a String `color` ColorProp (e.g. `red` or `#ff0000`) to change the default spinner color (by default it is `"white"` for high contrast on the default `overlayColor`; see below)
* You can control visibility of the spinner using the Boolean prop `visible` (Boolean, by default it is `false`)
* To change the color of the overlay, pass a ColorProp as the `overlayColor` prop (e.g. `'rgba(0,0,0,0.25)'`)
* Optional text field, activate by passing textContent Prop and style by passing textStyle Prop
* You can also pass a custom view to act as activity indicator.
| Property | Type | Default | Description |
| ------------- |----------------| --------|-------------|
| cancelable | `boolean` | `true` | **Android**: If set to false, it will prevent spinner from hiding when pressing the hardware back button.|
| color | `string` | `white` | Changes the spinner's color (example values are `red`, `#ff0000`, etc). For adjusting the contrast see `overlayColor` prop below.|
| overlayColor | `string` | `rgba(0, 0, 0, 0.25)` | Changes the color of the overlay.|
| size | `small`, `normal`, `large` | `large ` | Sets the spinner's size. No other cross-platform sizes are supported right now.|
| textContent | `string` | `""` | Optional text field to be shown.|
| textStyle | `style` | `-` | The style to be applied to the `<Text>` that displays the `textContent`.|
| visible | `boolean` | `false` | Controls the visibility of the spinner.|

You can also add a child view to act as a custom activity indicator.

```js
import React, { View, Text } from 'react-native';
Expand Down
10 changes: 9 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export default class Spinner extends React.Component {

static propTypes = {
visible: React.PropTypes.bool,
cancelable: React.PropTypes.bool,
textContent: React.PropTypes.string,
color: React.PropTypes.string,
size: React.PropTypes.oneOf(SIZES),
Expand All @@ -80,6 +81,7 @@ export default class Spinner extends React.Component {

static defaultProps = {
visible: false,
cancelable: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

@niftylettuce WDYT? IMO, this should be false by default to not introduce a braking change.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SandroMachado I agree, should be false and we should update Readme too if it isn't already
Copy link
Contributor

Choose a reason for hiding this comment

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

True. 👍
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SandroMachado @niftylettuce There are 2 main reasons I would vote against setting the default value to false:

  1. By setting the default value to false you 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 set cancelable:true.
  2. The native Android Dialog is by default cancelable.

If you still insist, I can change it though.
Copy link
Contributor

Choose a reason for hiding this comment

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

@gsavvid you are right. Can you please only update the README.md with this new prop?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great work!
textContent: "",
color: 'white',
size: 'large', // 'normal',
Expand All @@ -95,6 +97,12 @@ export default class Spinner extends React.Component {
this.setState({ visible, textContent });
}

_handleOnRequestClose() {
if (this.props.cancelable) {
this.close();
}
}

_renderDefaultContent() {
return (
<View style={styles.background}>
Expand Down Expand Up @@ -128,7 +136,7 @@ export default class Spinner extends React.Component {

return (
<Modal
onRequestClose={() => this.close()}
onRequestClose={() => this._handleOnRequestClose()}
supportedOrientations={['landscape', 'portrait']}
transparent
visible={visible}>
Expand Down