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

Fix Issue 10718: Add iOS support for progressViewOffset #30737

Conversation

davidbiedenbach
Copy link
Contributor

@davidbiedenbach davidbiedenbach commented Jan 14, 2021

Summary

Fixes #10718, bringing progressViewOffset support to iOS.

Thanks to @Taylor123 for the initial PR upon which this fix is based.

Changelog

[iOS] [Fix] - progressViewOffset prop of RefreshControl and VirtualizedList now works on iOS

Test Plan

Tested with quick-and-dirty sample app.

progressViewOffset-iOS

Documentation

The corresponding documentation update PR can be found here.

@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 14, 2021
@pull-bot
Copy link

pull-bot commented Jan 14, 2021

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 74af0db

@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Jan 14, 2021
@analysis-bot
Copy link

analysis-bot commented Jan 14, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: f276214

@analysis-bot
Copy link

analysis-bot commented Jan 14, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,896,647 -3,212
android hermes armeabi-v7a 8,394,383 -9,543
android hermes x86 9,384,976 -1,801
android hermes x86_64 9,330,068 -2,292
android jsc arm64-v8a 10,348,436 -3,246
android jsc armeabi-v7a 9,829,289 -9,559
android jsc x86 10,398,285 -1,814
android jsc x86_64 10,983,622 -2,316

Base commit: b32e996

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. Run yarn lint --fix to automatically fix problems.

@davidbiedenbach
Copy link
Contributor Author

davidbiedenbach commented Jan 21, 2021

Hi @hramos , I see in the comments of #10718 that you're willing to re-open the issue if a PR is submitted. Here's the PR, can you re-open the issue? =)

@davidbiedenbach davidbiedenbach changed the title Fix Issue 10718: Adds iOS support for progressViewOffset Fix Issue 10718: Add iOS support for progressViewOffset Feb 3, 2021
@andreialecu
Copy link

Ping @hramos:

happy to re-open the issue if you are ready to send a PR.
#10718 (comment)

@davidbiedenbach
Copy link
Contributor Author

Hi @sammy-SC , since you reviewed #30117, would you be able to take a look at this one? We've been using it in a production app and it's working great for us. I'd love to see this get merged sometime.

@sammy-SC
Copy link
Contributor

Hi @sammy-SC , since you reviewed #30117, would you be able to take a look at this one? We've been using it in a production app and it's working great for us. I'd love to see this get merged sometime.

Hello @davidbiedenbach,

thanks for the PR! I'll take a look on Monday :)

Copy link
Contributor

@sammy-SC sammy-SC left a comment

Choose a reason for hiding this comment

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

Could you copy paste code for the test app? Just the JavaScript part is sufficient.

Thank you!

@@ -25,6 +25,7 @@ - (UIView *)view
RCT_EXPORT_VIEW_PROPERTY(tintColor, UIColor)
RCT_EXPORT_VIEW_PROPERTY(title, NSString)
RCT_EXPORT_VIEW_PROPERTY(titleColor, UIColor)
RCT_EXPORT_VIEW_PROPERTY(progressViewOffset, float)
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 make this into CGFloat. It better expresses the use of the property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks for the feedback!

@davidbiedenbach
Copy link
Contributor Author

@sammy-SC here's the test code. Thanks again!

import React from 'react';
import {
  StyleSheet,
  FlatList,
  View,
  Text,
  Button,
} from 'react-native';

import {
  Colors,
} from 'react-native/Libraries/NewAppScreen';

const { useState, useCallback } = React;

const App: () => React$Node = () => {
  
  const [height, setHeight] = useState(50);
  const [refreshing, setRefreshing] = useState(false);
  const onRefresh = useCallback(()=>{
    setRefreshing(true);
    setTimeout(()=>setRefreshing(false), 3000);
  },[setRefreshing]);
  const onPressUp = useCallback(()=>{
    if (height > 0) {
      setHeight(height - 50);
    }
  },[height, setHeight]);
  const onPressDown = useCallback(()=>{
    setHeight(height + 50);
  },[height, setHeight]);
  const renderItem=useCallback(({item, index})=> {
    if (index === 0) {
      return (<View style={{height: 100, backgroundColor:'#dfdfdf'}}><Button title="^" onPress={onPressUp}>^</Button><Button title="v" onPress={onPressDown}>v</Button></View>)
    }
    return <View style={{height: 100, backgroundColor: index % 2 === 0 ? '#ffff00' : '#00FFFF'}}/>
  }, [onPressUp, onPressDown]);
  return (
    <>
      <View style={{position:'absolute', top: 100, bottom: 0, left: 0, right: 0}}>
        <FlatList 
          data={[0,1,2,3,4,5,6,7,8,9]}
          renderItem={renderItem}
          style={{backgroundColor: 'green'}}
          contentContainerStyle={{paddingTop: height}}
          progressViewOffset={height}
          onRefresh={onRefresh}
          refreshing={refreshing}
        />
        {height > 0 && <View style={{position: 'absolute', height, width: '100%', backgroundColor:'#cfcfcfaf', justifyContent:'center', alignItems:'center'}}><Text>This is an Absolute Header</Text></View>}
        </View>
    </>
  );
};

const styles = StyleSheet.create({
  scrollView: {
    backgroundColor: Colors.lighter,
  },
  engine: {
    position: 'absolute',
    right: 0,
  },
  body: {
    backgroundColor: Colors.white,
  },
  sectionContainer: {
    marginTop: 32,
    paddingHorizontal: 24,
  },
  sectionTitle: {
    fontSize: 24,
    fontWeight: '600',
    color: Colors.black,
  },
  sectionDescription: {
    marginTop: 8,
    fontSize: 18,
    fontWeight: '400',
    color: Colors.dark,
  },
  highlight: {
    fontWeight: '700',
  },
  footer: {
    color: Colors.dark,
    fontSize: 12,
    fontWeight: '600',
    padding: 4,
    paddingRight: 12,
    textAlign: 'right',
  },
});

export default App;

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.

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

Copy link
Contributor

@sammy-SC sammy-SC left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

// offset in the view hierarchy, and that progressViewOffset is not inadvertently applied
// multiple times.
UIView *scrollView = self.superview;
UIView *target = scrollView == nil ? nil : scrollView.superview;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a tip about obj-c :) Btw you can just call scrollView.superview. In case scrollView is nil, nil is returned from the entire expression. No need to handle nil case explicitly.

I'll change 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.

Ohhh. You're right, I completely forgot that dot syntax is just sending a message to a property getter here, isn't it? I'll try to remember that for next time. Thanks for the tip (and the correction)!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sending a message to nil does nothing in Obj-c. It is a feature, not a bug :)

I'll merge this on Monday.

Thank you.

@facebook-github-bot
Copy link
Contributor

@sammy-SC merged this pull request in 310a6bc.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 11, 2021
savv pushed a commit to savv/react-native-savv that referenced this pull request May 18, 2021
Summary:
Fixes facebook#10718, bringing `progressViewOffset` support to iOS.

Thanks to Taylor123 for the initial PR upon which this fix is based.

## Changelog

[iOS] [Fix] - `progressViewOffset` prop of `RefreshControl` and `VirtualizedList` now works on iOS

Pull Request resolved: facebook#30737

Test Plan:
Tested with quick-and-dirty sample app.

![progressViewOffset-iOS](https://user-images.githubusercontent.com/1563532/104526540-82fe1d80-55b7-11eb-9f99-e025bedf4874.gif)

## Documentation

The corresponding documentation update PR can be found [here](facebook/react-native-website#2441).

Reviewed By: kacieb

Differential Revision: D26813977

Pulled By: sammy-SC

fbshipit-source-id: 45cc5a647d70e44a29c6391b7586cb41ca011bef

(cherry picked from commit 310a6bc)
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. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offsetting RefreshControl on iOS via progressViewOffset
8 participants