enhancement - allow custom reusable cells

[nyura123]

backwards compatible, but intended to replace <Cell> custom cells; examples are in TableViewDemo (LargeTableExample and FirebaseExample).
Fixes issues with loading large data sets (#22) and re-rendering custom cells on data changes (#19).

Usage: instead of Cell's, use reactModuleForCell prop:

class LargeTableExample extends React.Component {
    render() {
        var numItems = 1000;
        var items = [];
        for (var i = 0; i < numItems; ++i) {
            items.push(i);
        }
        //Item can have any string/number fields that will be passed to your reactModuleForCell component.
        //However some fields are already used internally by the native table/cells: 
        //   height, selected, transparent, width, canEdit, canMove.
        return (
            <TableView reactModuleForCell="TableViewExampleCell" style={{flex:1}}>
                <Section label={numItems+" items"} arrow={true}>
                    {items.map((i)=><Item key={i+1} custField1={"item "+i} height={50}/>)}
                </Section>
            </TableView>
        );
    }
}
class TableViewExampleCell extends React.Component {
    render(){
        var style = {};
        if (this.props.data.height !== undefined) {
            style.height = this.props.data.height;
        } else {
            style.flex = 1;
        }
        if (this.props.data.backgroundColor !== undefined) {
            style.backgroundColor = this.props.data.backgroundColor;
        }
        return (<View style={style}><Text>{this.props.data.custField1}</Text></View>);
    }
}

//Need to register the cell component so it can be rendered as a root view - this is the key 
//to letting ios control the lifetime of the cells while react just renders into them on demand.
AppRegistry.registerComponent('TableViewExampleCell', () => TableViewExampleCell);

[radford-for-smpte]

Curious, does this resolve #46?

In my use cases, the ability to hide and/or remove cells is critical. How else can you perform search/filtering, etc?

[nyura123]

@Studio-Dude I believe it should fix #19 (and therefore #46) - I tried testing using dynamic data (from firebase) and it handled adding/deleting/updating rows fine for me.

[mosesoak]

This is cool, I'm going to give it a spin right now!

[nyura123]

@mosesoak cool! it does need those 2 one-line changes in react-native.

On Feb 8, 2016, at 4:03 PM, moses gunesch [email protected] wrote:

This is cool, I'm going to give it a spin right now!

—
Reply to this email directly or view it on GitHub.

[aksonov]

@nyura123 "Major caveat: 2 changes are needed in react-native RCTRootView.m per the comments in RNReactModuleCell.h." - it is very major caveat. It makes this PR almost useless, because uses non-public API, so it could be easily broken for next versions of React Native. Is there any other way to avoid this?

Btw, we could require enhancing of React Native API if we will have strong reasoning for that - you could create issue in their github or add it to https://productpains.com. If community supports it, Facebook team will done it.

[nyura123]

@aksonov I figured out a way around it

[chetstone]

@nyura123 what is the way around it? It looks like you have added an assertion to test if the required lines have been added to AppDelegate.m. Or have you found a way to do it so that those additions are not required? If so it doesn't appear to be in the PR commits yet.

[nyura123]

@chetstone the react-native changes are no longer needed - instead an app using this feature would need to set the bridge it wants to be used by RNReactModuleCell root views. That's what is being asserted (only if you're using this feature, that is reactModuleForCell prop).

AppDelegate.m:

  //Save main bridge so that RNTableView could access our bridge to create its RNReactModuleCells
  [[RNAppGlobals sharedInstance] setAppBridge:rootView.bridge];

The problem was that I wanted the cell root views to use my global bridge I create in AppDelegate. But that bridge doesn't seem to be exposed/stored anywhere, instead you only have access to bridge.batchedBridge (for ex. through self.bridge in RNTableViewManager). If you try to pass this batchedBridge to RCTRootView initWithBridge, it will break here:

- (instancetype)initWithBridge:(RCTBridge *)bridge
                    moduleName:(NSString *)moduleName
             initialProperties:(NSDictionary *)initialProperties {
...
    if (!_bridge.loading) {
      [self bundleFinishedLoading:_bridge.batchedBridge]; //_bridge is already a batchedBridge, _bridge.batchedBridge is nil!
    }

so the solution was to just manually share the bridge using a singleton. Long-term I think the react-native code above might need to be fixed by replacing the _bridge.batchedBridge with _bridge, and same for the RCTRootView::setAppProperties which has the same issue.

[chetstone]

On Thu, Feb 25, 2016 at 4:25 PM, nyura123 [email protected] wrote:

RNReactModuleCell

Thanks for the reply. I don't understand everything about native bridges,
but I think what you're saying is this:

1. If you want to use RNReactModuleCell with your current implementation
   you will need to add those lines to AppDelegate.m
2. Modifying AppDelegate.m is not considered modifying react-native
   code. (Because during the course of development one is frequently modifying
   that file anyway, and possible breakage from RN upgrades is prevented by an
   assertion.) When you say you don't have to modify RN code, you're talking
   about the deep internals. I think I agree with that.

I'll give it a try. Thanks.

[nyura123]

@chetstone, curious if you had any luck with this?

[chetstone]

Yes, so far so good. I've modified the Custom Editing Example to try out sample data similar to what I need to do in my app. One thing I noticed, which may or may not be an issue is that when you add an item to the list, the other items in the list are not re-rendered. This is a potential problem because I need to display a relative time ("a few seconds ago")... It's probably OK because normally the list only needs to be re-rendered when the screen is activated, and that should happen automatically, but I'll need to do further testing in my app.

And also my list is in reverse order, with the newest entry on top. When I try that, all re-render when I push the new one. Can I use shouldComponentUpdate() with this to improve rendering time?

I also had a crash when clicking the Edit button when the list is empty. The problem is this line, which you recently changed:

            this.preEditData = this.state.data.slice(0);

I was going to send you a PR later today but maybe it's easier for you to just fix it.

This PR was a lot of work, I appreciate it. @aksonov, what do you think? I think the idea of requiring a small change in AppDelegate.m to use the feature is acceptable, and it addresses several issues.

It needs docs, and if they are too much or too confusing to put in the README, perhaps the details could be on the Wiki with a pointer from the README.

Oh, one other thing I noticed that seems a little strange is the message in xcode/Chrome:

Running application "TableViewExampleCell2" with appParams: {...}. __DEV__ === true, development-level warning are ON, performance optimizations are OFF

I guess it's expected but a little disconcerting to be apparently running 2 apps at once. Any possible gotchas here?

[nyura123]

@chetstone, thanks - yep, I'll fix the "slice" bug in the test example.

Regarding "Running application" logging, you are actually running many different applications, one for each cell. An application is whatever renders into a root view, and in this case, each cell is a root view. I'm guessing in production mode, you wouldn't see this logging, but I haven't tried it.
This PR was indeed a lot of work :) - mainly reading react-native code to understand how we could have both iOS cell recycling and yet use react rendering. Eventually I realized that the root view functionality already allows us to do exactly that - there might be some gotchas with that but so far I haven't seen them.

For re-rendering existing items on adding a new item, maybe explicitly passing a "time" field (or some other field to trigger rendering) in each Item would help?
var now = new Date().getTime()

[aksonov]

@nyura123 So AppDelegate should be changed only when user wants to use custom cells? I don't want to break compatibility with current API.

Could you also change demo to use your feature?

[nyura123]

Yes, only if you use reactModuleForCell prop. And to clarify the prop should only be used with Items, not Cells. Cells behavior works the same as before.
I added 4-5 examples to TableViewDemo index.ios.js already.

[chetstone]

I got this implemented in my project. Works great! Interestingly, performance seems to be much more responsive than ListView. I had to limit my list to 50 items with ListView to get decent performance, now I can view 500 no problem.
@nyura123 could you please take a look at my documentation PR?

[nyura123]

@chetstone, thanks for adding the documentation - looks great to me! Great to hear it's working for you!

[chetstone]

Just discovered that on larger screens the Items don't use the full width of the screen. Any idea on what to look for? I don't see anything in my view styles that could be limiting the width.

Here's what it looks like on a smaller screen. OK.

iPhone 5s simulator

And this on a larger screen:

iPhone6+ device (same on simulator)

[chetstone]

Same thing happens with TableViewDemo.

iPhone 6+ simulator:

[nyura123]

I am not 100% sure but it could be because it renders into the content view
(which doesn't take up the full width of the cell). Have you tried setting
the "width" param? Do you see the same issue with regular
(non-reactModuleForCell) Items?

On Mon, Apr 4, 2016 at 7:01 PM, Chester Wood [email protected]
wrote:

Same thing happens with TableViewDemo.

iPhone 6+ simulator:

[image: simulator screen shot apr 4 2016 17 58 35]
https://cloud.githubusercontent.com/assets/976343/14267614/306e194a-fa8f-11e5-97e4-f104c169e0e2.png

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#76 (comment)

[chetstone]

No issue with non-reactModuleForCell items. Explicitly setting the width parameter does help (red cell in the screen shot below). Can you suggest the easiest way to query for the proper width for the device?

Note that the Inspector is pretty useless, "Inspect/Perf" buttons show up on every reactModuleForCell component.

[chetstone]

Setting width statically seems to work, but I can't get the cells to display at the proper width when changing orientation. In my reactModuleForCell component I added:

    if (this.props.data.width !== undefined) {
      style.width = this.props.data.width;
    } else {
      style.flex = 1;
    }
    debug("TableViewCell width: ", style.width);  

Console output shows that style.width is being changed correctly (via a react-native-orientation listener) when orientation changes, but the cells do not respond, or respond only rarely, then get 'stuck' in the new width for more orientation changes.

It appears that the native cells are being rendered at the right width, because native-rendered stuff like the Delete button and Disclosure indicator are correctly located on the right margin, but the width information is not being sent back to the React-native module. I think it would be preferable for it to work this way (as it seems to be done, according to the comments in the example code, for cell height). Sending width down via props is not working well.

[nyura123]

Maybe it's better to always have the root view take up the full width of the its parent contentView (which takes care of resizing on orientation changes).
Then ideally you wouldn't need to set width prop at all, and if you do, it would only affect the react view (the rootView's child).

Setting autoresizingMask seems to do the trick, RNReactModuleCell.m. It still leaves space for the disclosure indicator if it's there, otherwise fills the full width.

-(void)setUpAndConfigure:(NSDictionary*)data bridge:(RCTBridge*)bridge indexPath:(NSIndexPath*)indexPath reactModule:(NSString*)reactModule{
    NSDictionary *props = [self toProps:data indexPath:indexPath];
    if (_rootView == nil) {
        //Create the mini react app that will populate our cell. This will be called from cellForRowAtIndexPath
        _rootView = [[RCTRootView alloc] initWithBridge:bridge moduleName:reactModule initialProperties:props];
        [self.contentView addSubview:_rootView];
        _rootView.frame = self.contentView.bounds;
        //Fill up the full height/width of the content view.
       _rootView.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight;
...

[chetstone]

That works great. Thank you!

[michaelphines]

@nyura123 Does this support flexible cell height based on the react module's height? My cells all overlap:

[nyura123]

@michaelphines have you tried setting height prop on the Item?

[michaelphines]

@nyura123 With the height property I can only set a static height, right? The problem is that the comments are of variable length in my app. For example, I can set the height to 100, but then shorter comments don't take up all the space, and longer comments overflow. This doesn't happen with Cell, but unfortunately using Cells is far too slow.

Edit: sorry about the NSFW language in the previous screenshot

[nyura123]

(haha, looks like an interesting conversation...)
You could try to calculate each Item's height based on the size of the comment, but it would be hard to do it correctly for all screen sizes/orientations. I'll try to see if height can be auto-sized like for Cells.