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

Ability to provide custom ScrollableComponent ref prop (ie innerRef) #273

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@slorber
Collaborator

slorber commented Jul 2, 2018

This permits to integrate more easily with other existing ScrollView wrappers/HOC's

To give more insight on my usecase, here's real code of my app (that will work once this PR is merged).
Note that I use glamorous ScrollView on purpose so in any case I need to pass it innerRef prop, not ref.

import React from 'react';
import { listenToKeyboardEvents } from 'react-native-keyboard-aware-scroll-view';
import { View, ScrollView } from 'glamorous-native';
import { ScrollIntoViewWrapper } from 'react-native-scroll-into-view';
import { AnimatedScrollView } from 'common/components/utils/GlamorousUtils';
import { compose } from 'recompose';

const makeKeyboardAware = Comp => {
  const debugOnAndroid = __DEV__ && true;
  const Wrapped = listenToKeyboardEvents(Comp);
  // Need to provide keyboard-aware hoc conf by props for now :(
  // See https://github.com/APSL/react-native-keyboard-aware-scroll-view/issues/272
  Wrapped.defaultProps = {
    refPropName: 'innerRef',
    enableOnAndroid: debugOnAndroid,
  };
  return Wrapped;
};

// We implement a custom scrollview because:
// - we need to ensure ios inputs remain visible after keyboard apparition (works by default on android)
// - we need equivalent of "element.scrollIntoView()" DOM function for some cases of the app
const CustomScrollView = compose(
  makeKeyboardAware,
  ScrollIntoViewWrapper({ refPropName: 'innerRef' }),
)(ScrollView);

export default CustomScrollView;

export const CustomScrollViewAnimated = compose(
  makeKeyboardAware,
  ScrollIntoViewWrapper({ refPropName: 'innerRef' }),
)(AnimatedScrollView);

slorber added some commits Jul 2, 2018

@slorber slorber referenced this pull request Jul 2, 2018

Closed

Ability to configure HOC #272

@slorber

This comment has been minimized.

Collaborator

slorber commented Jul 11, 2018

@alvaromb can you please merge this? I've tested and it's working and is retrocompatible with current behavior

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 12, 2018

@slorber looking into this change atm.

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 12, 2018

I would be more comfortable by applying your change and moving from HOC to HOF (high order function).

How do you feel about that, @slorber?

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 12, 2018

If that's ok for you, I can provide HOF implementation today.

@slorber

This comment has been minimized.

Collaborator

slorber commented Jul 12, 2018

not sure what you mean, but if you propose listenToKeyboardEvents(config)(Comp) instead (or in addition) of props like it is now, that would be fine for me yes

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 12, 2018

Yes! that's my proposal!

@slorber

This comment has been minimized.

Collaborator

slorber commented Jul 12, 2018

great please ping me when you have a new version and I'll test it

@slorber

This comment has been minimized.

Collaborator

slorber commented Jul 18, 2018

@alvaromb have you been able to work on this? I'm waiting for publishing my app ;)

I can work on a PR myself if you want, as long as you will merge it rapidly (otherwise I'd have to publish a fork)

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 18, 2018

@slorber I'll have a slot to implement that tomorrow. If you can send a PR before, I'll get that merged today.

@slorber

This comment has been minimized.

Collaborator

slorber commented Jul 18, 2018

I can wait some days :)

Tell me if you didn't find time to do it tomorrow and I'll do it.
If so please do share your ideas, otherwise I'd implement something close to this API like the one I have done here: https://github.com/slorber/react-native-scroll-into-view/blob/master/scrollIntoView.js#L263

export const ScrollIntoViewWrapper = configOrComp => {
  if ( typeof configOrComp === "object" ) {
    return Comp => ScrollIntoViewWrapperHOC(Comp,configOrComp);
  }
  else {
    return ScrollIntoViewWrapperHOC(configOrComp);
  }
};
@slorber

This comment has been minimized.

Collaborator

slorber commented Jul 30, 2018

@alvaromb unless you want to do it soon, I'm going to make another PR this week

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 31, 2018

@slorber we're in the middle of a very very important release and we do not have time right now to handle this reviews. I've added you as a collaborator, how do you feel about being collab & having release access?

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 31, 2018

I think you should be able to merge PR's even if the master branch is protected. Ping me if not.

@slorber

This comment has been minimized.

Collaborator

slorber commented Jul 31, 2018

Thanks, I'll make a PR for that probably today or tomorrow. If you want to to release i'm also @slorber on npm

@alvaromb

This comment has been minimized.

Member

alvaromb commented Jul 31, 2018

I've also added you on npm. Please don't forget to write Release Notes: https://github.com/APSL/react-native-keyboard-aware-scroll-view/releases

@slorber

This comment has been minimized.

Collaborator

slorber commented Aug 1, 2018

superseded by #288

@slorber slorber closed this Aug 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment