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

Delegate to ProgressBarAndroid from ActivityIndicator on Android, instead of the other way around #16435

Closed
wants to merge 1 commit into from

Conversation

brentvatne
Copy link
Collaborator

@brentvatne brentvatne commented Oct 17, 2017

Motivation

ProgressBarAndroid regressed after fixing a bug in ccddbf8

Test Plan

Run this gist on a new project with this code: https://gist.github.com/brentvatne/a0b57e5bbae1bd2cf76765ea27f077af

Notice that you will see:

screen shot 2017-10-17 at 11 06 03 am

hmmm... doesn't seem right 🤔

With the patch in this PR applied, you will see:

screen shot 2017-10-17 at 11 01 38 am

oh! there we go 😄

Release Notes

[ANDROID] [BUGFIX] [ProgressBarAndroid] - Fix regression in ProgressBarAndroid which limited styleAttr to only Regular.

@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 Oct 17, 2017
@pull-bot
Copy link

pull-bot commented Oct 17, 2017

@facebook-github-bot label Core Team

Generated by 🚫 dangerJS

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Did a quick smoke-test in Catalyst and this looks good to me. 👍

Thanks for this PR!

@ide
Copy link
Contributor

ide commented Oct 17, 2017

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Oct 17, 2017
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.

@ide is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ide ide deleted the fix-progress-bar-android branch October 18, 2017 04:11
ide pushed a commit that referenced this pull request Oct 18, 2017
…tead of the other way around

Summary:
`ProgressBarAndroid` regressed after fixing a bug in ccddbf8

Run this gist on a new project with this code: https://gist.github.com/brentvatne/a0b57e5bbae1bd2cf76765ea27f077af

Notice that you will see:

<img width="642" alt="screen shot 2017-10-17 at 11 06 03 am" src="https://user-images.githubusercontent.com/90494/31681142-3437a95a-b32b-11e7-85d3-c29bfbfe591e.png">

hmmm... doesn't seem right �

With the patch in this PR applied, you will see:

<img width="642" alt="screen shot 2017-10-17 at 11 01 38 am" src="https://user-images.githubusercontent.com/90494/31680950-b0c1805a-b32a-11e7-909e-42cdf478da56.png">

oh! there we go 😄

[ANDROID] [BUGFIX] [ProgressBarAndroid] - Fix regression in ProgressBarAndroid which limited `styleAttr` to only `Regular`.
Closes #16435

Differential Revision: D6080118

Pulled By: hramos

fbshipit-source-id: 537ee2c96deedd7b2e75ff3dbdefc1506812f3f3
brentvatne added a commit to expo/react-native that referenced this pull request Oct 18, 2017
… 'Horizontal' in ProgressBarAndroid

- We delegate to ProgressBarAndroid in ActivityIndicator as of facebook#16435 so this warning is thrown constantly.
ide pushed a commit to expo/react-native that referenced this pull request Oct 18, 2017
… 'Horizontal' in ProgressBarAndroid

- We delegate to ProgressBarAndroid in ActivityIndicator as of facebook#16435 so this warning is thrown constantly.
@bvaughn
Copy link
Contributor

bvaughn commented Oct 18, 2017

I didn't notice this while reviewing the code (since it wasn't part of the diff) but it looks like this changed introduced a bunch of deprecation warnings due to ProgressBarAndroid:

  componentDidMount() {
    if (this.props.indeterminate && this.props.styleAttr !== 'Horizontal') {
      console.warn(
        'Circular indeterminate `ProgressBarAndroid`' +
        'is deprecated. Use `ActivityIndicator` instead.'
      );
    }
  }

@bvaughn
Copy link
Contributor

bvaughn commented Oct 18, 2017

I will follow up on this internally.

Just a quick note though that for the time being, this change has been reverted (within Facebook) because it caused a bunch of yellow box deprecation warnings (the one I pointed out above) to be shown. I think the correct fix at this point is to re-land this change along with removing that deprecation warning, but I want to run this by others first to make sure there's no concerns with it.

@ide
Copy link
Contributor

ide commented Oct 18, 2017

Brent has a commit up to fix the warning too. The longer-term fix we were thinking about is to split into three components, two that are public (ActivityIndicator and ProgressBar) that use a private third component underneath.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 18, 2017

Might be easiest to just let me do that internally along with the rollback of the rollback, if you don't mind. (I've already started landing the diff anyway.)

bvaughn referenced this pull request Oct 18, 2017
…om ActivityIndicator on Android, instead of the other way around

Differential Revision: D6080118

fbshipit-source-id: efd75bbcc07de084213d3791520006090001364d
@bvaughn
Copy link
Contributor

bvaughn commented Oct 18, 2017

Okay. The internal diff (D6092598) has landed re-adding this change and removing the deprecation message. It should sync to GitHub soon.

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.

None yet

5 participants