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

Activity indicator: add size prop #8935

Closed

Conversation

fadils
Copy link
Contributor

@fadils fadils commented Jul 21, 2016

motivation

Previously, size can only accept either 'small' or 'large'. And to obtain a custom size, scale transformation is used. This is to let users to possibly pass number value directly to define ActivityIndicator's size.

Test plan

I have also modified the current example to reflect the new size prop in action.

@ghost
Copy link

ghost commented Jul 21, 2016

By analyzing the blame information on this pull request, we identified @janicduplessis and @sebmarkbage to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 21, 2016
@fadils fadils changed the title Activity indicator/add size prop Activity indicator: add size prop Jul 21, 2016
@fadils fadils force-pushed the ActivityIndicator/add-size-prop branch from f82158b to 369e201 Compare July 21, 2016 01:55
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 21, 2016
}
},
{
title: 'Custom size (size: 31, scale transform: 1.5)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the scale example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do @janicduplessis

@janicduplessis
Copy link
Contributor

Have you tested this change on both iOS and Android? Also does changing the size like this actually scales up the indicator (as far as I remember it didn't on iOS and it is why we had use a scale transform).

@@ -0,0 +1,15 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a separate docs page just for indicator size? Can we not include it in the same documentation as ActivityIndicator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the case is because we use custom propType checker instead. Since I'm gonna revert it, this doc is will be better left out. Thanks!

@ghost ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 21, 2016
@fadils fadils force-pushed the ActivityIndicator/add-size-prop branch from 369e201 to 594eded Compare July 22, 2016 04:31
@fadils fadils force-pushed the ActivityIndicator/add-size-prop branch from 594eded to b0b27cd Compare July 22, 2016 04:35
render() {
return (
<ActivityIndicator
style={styles.centering}

Choose a reason for hiding this comment

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

number This type is incompatible with string

Choose a reason for hiding this comment

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

number This type is incompatible with string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@satya164 This is what I was talking about that flow keeps complaining, and thus I decided to create a custom propType checker instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fadils Seems Flow is confused regarding the types. Can you add annotation to getDefaultProps so that flow doesn't try to infer the type and see if it fixes the issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

type IndicatorSize = number | 'small' | 'large';

type DefaultProps = {
  animating: boolean;
  color: any;
  hidesWhenStopped: boolean;
  size: IndicatorSize;
}

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 22, 2016
@fadils
Copy link
Contributor Author

fadils commented Jul 22, 2016

@janicduplessis, @satya164 not yet working on iOS. Should we create an Android specific prop for now? Maybe like customSize with @platform android ?

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 22, 2016
@janicduplessis
Copy link
Contributor

Let's add a comment that says that passing a number to the size prop is only supported on Android in the prop documentation.

@fadils
Copy link
Contributor Author

fadils commented Jul 22, 2016

Alright. Any idea to make flow happy with this and consequently pass the CI?

@satya164
Copy link
Contributor

Will check the Flow thing in a while.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 22, 2016
Previously, size can only accept either 'small' or 'large'.
And to obtain a custom size, scale transformation is used.
This is to let users to possibly pass number value to define
ActivityIndicator's size.

Not yet supported on iOS.
@fadils fadils force-pushed the ActivityIndicator/add-size-prop branch from b0b27cd to 168666b Compare July 23, 2016 00:26
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 23, 2016
@fadils
Copy link
Contributor Author

fadils commented Jul 23, 2016

@satya164 the annotation works. Awesome!
@janicduplessis comments added. Examples updated too.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 23, 2016
];

if (Platform.OS === 'android') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the platform prop in the example object to avoid having to do this.

{
  platform: 'android',
  title: '',
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Didn't know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janicduplessis Previously, you suggested that we erase example that uses transformation.
I was thinking that if we put

{
  platform: 'ios',
  ...
}

then it would be a bit misleading as actually transformation can be done in Android as well.

If we didn't use platform, then the example will still be shown in Android, along with the custom size (with number) examples.

Any input?

Should we just keep it with something like this:

{
   title: 'Custom size (transform scale: 1.5)',
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the scale example and just add one with a custom size that is android only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Thanks!

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 23, 2016
@fadils fadils force-pushed the ActivityIndicator/add-size-prop branch from 168666b to 384f654 Compare July 23, 2016 05:42
@@ -24,6 +24,7 @@

const React = require('react');
const ReactNative = require('react-native');
const Platform = require('Platform');

Choose a reason for hiding this comment

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

no-unused-vars: 'Platform' is defined but never used

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 23, 2016
@fadils fadils force-pushed the ActivityIndicator/add-size-prop branch from 384f654 to a86576d Compare July 23, 2016 05:59
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 23, 2016
@fadils
Copy link
Contributor Author

fadils commented Jul 28, 2016

@satya164 @janicduplessis is there anything else to ship this PR? I would be more than happy to add/change anything.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 28, 2016
@satya164
Copy link
Contributor

@fadils Thanks for the ping.

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 28, 2016
@ghost
Copy link

ghost commented Jul 28, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 28, 2016
@ghost ghost closed this in 63d15af Jul 28, 2016
bartolkaruza pushed a commit to immidi/react-native that referenced this pull request Aug 3, 2016
Summary:
**motivation**

Previously, size can only accept either 'small' or 'large'. And to obtain a custom size, scale transformation is used. This is to let users to possibly pass number value directly to define ActivityIndicator's size.

**Test plan**

I have also modified the current example to reflect the new size prop in action.
Closes facebook#8935

Differential Revision: D3637910

fbshipit-source-id: 6b8e1d4504964916df327b2d3eaaef1bb8cd5112
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Aug 11, 2016
Summary:
**motivation**

Previously, size can only accept either 'small' or 'large'. And to obtain a custom size, scale transformation is used. This is to let users to possibly pass number value directly to define ActivityIndicator's size.

**Test plan**

I have also modified the current example to reflect the new size prop in action.
Closes facebook/react-native#8935

Differential Revision: D3637910

fbshipit-source-id: 6b8e1d4504964916df327b2d3eaaef1bb8cd5112
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
**motivation**

Previously, size can only accept either 'small' or 'large'. And to obtain a custom size, scale transformation is used. This is to let users to possibly pass number value directly to define ActivityIndicator's size.

**Test plan**

I have also modified the current example to reflect the new size prop in action.
Closes facebook#8935

Differential Revision: D3637910

fbshipit-source-id: 6b8e1d4504964916df327b2d3eaaef1bb8cd5112
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants