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

Moved requireNativeComponent to a new file #23084

Closed
wants to merge 4 commits into from
Closed

Moved requireNativeComponent to a new file #23084

wants to merge 4 commits into from

Conversation

ajstrand
Copy link

@ajstrand ajstrand commented Jan 21, 2019

Changelog:

[Android] [Changed] - I did some work for issue #22990. All the imports connected to requireNativeComponent in ProgressBarAndroid were moved to a separate file.

Test Plan:

  • yarn test

  • yarn flow

  • yarn lint

Question:

@TheSavior Would the types seen here in ProgressBarAndroid.android.js need to be duplicated or moved into my new file? I wasn't sure if they are meant to be, considering they are being exported.

export type ProgressBarAndroidProps = $ReadOnly<{|
  ...ViewProps,

  /**
   * Style of the ProgressBar and whether it shows indeterminate progress (e.g. spinner).
   *
   * `indeterminate` can only be false if `styleAttr` is Horizontal, and requires a
   * `progress` value.
   */
  ...
    | {|
        styleAttr: 'Horizontal',
        indeterminate: false,
        progress: number,
      |}
    | {|
        typeAttr:
          | 'Horizontal'
          | 'Normal'
          | 'Small'
          | 'Large'
          | 'Inverse'
          | 'SmallInverse'
          | 'LargeInverse',
        indeterminate: true,
      |},
  /**
   * Whether to show the ProgressBar (true, the default) or hide it (false).
   */
  animating?: ?boolean,
  /**
   * Color of the progress bar.
   */
  color?: ?string,
  /**
   * Used to locate this view in end-to-end tests.
   */
  testID?: ?string,
|}>;

@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 Jan 21, 2019
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.

@elicwhite
Copy link
Member

Thanks for the PR!

Yeah, it would be great if you could duplicate the type and put it in the new file, typing the requireNativeComponent call.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.

@ajstrand
Copy link
Author

@TheSavior I've added the flow type for the native component. All of the yarn tests(flow, lint, test), still pass locally. Would there be anything else I would need to do for this PR?

I've tried testing my changes in the RNTester App, but the app crashes when I try to run it on an emulator. I don't seem to get any sort of error message either.

@elicwhite
Copy link
Member

Thanks for updating it. You should be able to get the ci/circleci: analyze build to pass for this PR. I think the android and ios ones are broken right now on master but the analyze one looks like it has valid flow errors from your PR.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Contributor

cpojer commented Jan 22, 2019

Thank you for your PR. It turns out that this was already implemented in #23068 before you submitted a PR. I'm sorry that this caused you to do extra work :(

@cpojer cpojer closed this Jan 22, 2019
@ajstrand
Copy link
Author

hey @cpojer it's ok. It wasa good exposure to react native and flow.

@ajstrand ajstrand deleted the Standalone-for-NativeComponents]-AndroidProgressBar.js branch January 23, 2019 00:55
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. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants