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

add withNavigationFocus HOC #3512

Merged
merged 10 commits into from Feb 17, 2018

Conversation

Projects
None yet
5 participants
@slorber
Contributor

slorber commented Feb 13, 2018

As discussed in other issues, we should be able to know easily which screen has the focus.

This HOC idea has been mentioned here by @satya164 and @ericvicenti

Here is a basic implementation using the new lifecycle hooks to provide this feature

@codecov-io

This comment has been minimized.

codecov-io commented Feb 13, 2018

Codecov Report

Merging #3512 into master will decrease coverage by 0.54%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3512      +/-   ##
==========================================
- Coverage    72.6%   72.05%   -0.55%     
==========================================
  Files          51       52       +1     
  Lines        1595     1607      +12     
==========================================
  Hits         1158     1158              
- Misses        437      449      +12
Impacted Files Coverage Δ
src/views/withNavigationFocus.js 0% <0%> (ø)
src/react-navigation.web.js 0% <0%> (ø) ⬆️
src/react-navigation.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69bca19...674389a. Read the comment docs.

slorber added some commits Feb 13, 2018

@slorber

This comment has been minimized.

Contributor

slorber commented Feb 13, 2018

just added an example to playground app: TabsWithNavigationFocus

@ericvicenti

Yes, this is exactly what I had in mind. Thanks for the contribution!!

};
componentDidMount() {
if (this.props.navigation) {

This comment has been minimized.

@ericvicenti

ericvicenti Feb 13, 2018

Contributor

I think it would be great if we could use the navigtion from context if the user forgets to pass it in as a prop. Take a look at the other HOC, withNavigation.

),
];
} else {
console.warn(

This comment has been minimized.

@ericvicenti

ericvicenti Feb 13, 2018

Contributor

Once we start accepting context from this HOC, I think it would best to have an invariant here instead of warning- things would need to be very broken.

<Component
{...this.props}
isFocused={this.state.isFocused}
ref={this.props.onRef}

This comment has been minimized.

@satya164

satya164 Feb 13, 2018

Collaborator

This will not work for function components. We need to check if the component is a function component or not before attaching the ref, and only attach the ref to class components.

On another note, I have seen a getWrappedInstance method to be a more common way to expose the wrapped component's ref, so maybe better to use it instead of this.props.onRef for better ecosystem compatibility.

This comment has been minimized.

@ericvicenti

ericvicenti Feb 13, 2018

Contributor

We already used onRef somewhere else cc @brentvatne (although I don't feel strongly either way)

This comment has been minimized.

@satya164

This comment has been minimized.

@slorber

slorber Feb 13, 2018

Contributor

I prefer getWrappedInstance compared to this.props.onRef as it is used in react-intl and react-redux (but it requires a {withRef: true} config on the HOC. I used onRef because it's already what is available on withNavigation so didn't want to use something different and keep the API consistent.

Also worth noting that I don't like much the api of react-intl as it does not play well with compose because the config is 2nd arg. I prefer allowing withNavigationFocus(config)(Comp) and withNavigationFocus(Comp) instead of withNavigationFocus(Comp,config). What do you think about it?

This comment has been minimized.

@satya164

satya164 Feb 13, 2018

Collaborator

it requires a {withRef: true} config on the HOC

Yeah, feels kinda unnecessary, but no idea why it's that way.

I prefer allowing withNavigationFocus(config)(Comp)

+1, but what config?

This comment has been minimized.

@satya164

satya164 Feb 13, 2018

Collaborator

Do we need really need a config for { withRef: true }? Isn't it enough to detect if a component is function component or class and do it automatically? Or there are other issues with attaching ref which I'm missing?

This comment has been minimized.

@satya164

satya164 Feb 13, 2018

Collaborator

onRef is used here

ref={this.props.onRef}

@slorber

This comment has been minimized.

Contributor

slorber commented Feb 13, 2018

I updated the PR to get navigation from both props and context

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Feb 13, 2018

Oh yay, flow errors! 😭

Error: js/TabsWithNavigationFocus.js:7
  7: import { TabNavigator, withNavigationFocus } from 'react-navigation';
                            ^^^^^^^^^^^^^^^^^^^ Named import from module `react-navigation`. This module has no named export called `withNavigationFocus`.

Error: js/TabsWithNavigationFocus.js:21
 21:       <SampleText>Tab {name}</SampleText>
           ^^^^^^^^^^^^ React children array. Could not decide which case to select
 12: const SampleText = ({ children }: { children?: ChildrenArray<*> }) => (
                                                    ^^^^^^^^^^^^^^^^ union type. See: js/SampleText.js:12
  Case 1 may work:
  223:   declare export type ChildrenArray<+T> = $ReadOnlyArray<ChildrenArray<T>> | T;
                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ read-only array type. See lib: /tmp/flow/flowlib_28386f1e/react.js:223
  But if it doesn't, case 2 looks promising too:
   12: const SampleText = ({ children }: { children?: ChildrenArray<*> }) => (
                                                                    ^ existential. See: js/SampleText.js:12
  Please provide additional annotation(s) to determine whether case 1 works (or consider merging it with case 2):
   21:       <SampleText>Tab {name}</SampleText>
                              ^^^^ name


Found 2 errors
error Command failed with exit code 2.
Exited with code 1

Looks like you need to add a definition for this new HOC in flow/react-navigation.js

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Feb 13, 2018

As for the getWrappedInstance vs props.onRef question, lets move that to a follow-up issue/PR/RFC, because we're already invested in props.onRef for now because of withNavigation. I don't have a strong preference either way, so I'll leave it up to y'all, but I believe that onRef requires a tiny bit less boilerplate for people.

@slorber

This comment has been minimized.

Contributor

slorber commented Feb 14, 2018

fixed the flow errors on playground, tell me if there's anything else to do

@brentvatne brentvatne merged commit c74f001 into react-navigation:master Feb 17, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 17, 2018

@slorber - can you open a PR on the website repo with an API reference for this? http://github.com/react-navigation/website

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 17, 2018

@slorber - it turns out that this isn't a reliable way to find the navigation focused state. it will only work if it is wrapping a navigation screen. if you add it to some component in the screen, and that component is rendered after the parent screen component is mounted, this will not work until you navigate to and away from the screen where it's rendered. this is because it depends on navigation events, and those are only fired when things change. we need some way to be able to reach in to the parent navigation state and determine if it is focused synchronously on mount, in the state initializer. cc @ericvicenti

@satya164

This comment has been minimized.

Collaborator

satya164 commented Feb 17, 2018

Maybe a this.props.navigation.isFocused() method will be helpful to get the current focused state? Users can get the initial focus state by using it in componentWillMount

@slorber

This comment has been minimized.

Contributor

slorber commented Feb 17, 2018

I understand that problem, do you have any solution to propose?

What about a new navigator config withNavigationFocus: true, and making the HOC private/lib internal. This way all screens will be wrapped by the HOC and we can be sure they won't be mounted after the focus event. This HOC makes mostly sense at the root so navigator config can be an appropriate place.

@satya164 I wouldn't use such isFocused function. I've used such child component querying parent from function prop and this get really confusing because if you use this on render (and be certain people will try that) you will be confused because the component won't render on focus change if the component is pure and function is stable. I tricked myself with that in the past.

@satya164

This comment has been minimized.

Collaborator

satya164 commented Feb 17, 2018

Another option is to always call the listener with the current value synchronously as soon as you attach it.

@slorber

This comment has been minimized.

Contributor

slorber commented Feb 17, 2018

I like this idea (it's what History.listen does to provide immediate history location), but shouldn't we create a onFocusChange or something instead of calling didFocus immediately?

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 23, 2018

I don't like calling didFocus immediately, this leads to IMO some confusing behaviors. I also don't like calling onFocusChange to solve the problem that I mentioned above because it's semantically incorrect. We should just expose isFocused(), and initialize the withNavigationFocus state from that.

};
state = {
isFocused: false,

This comment has been minimized.

@brentvatne

brentvatne Feb 23, 2018

Member

as per my most recent comment, if we exposed isFocused() then this would just be:

constructor(props, context) {
  super();
  const navigation = props.navigation || context.navigation;
  this.state = {
    isFocused: navigation.isFocused(),
  };
}

This comment has been minimized.

@slorber

slorber Feb 24, 2018

Contributor

So isFocused is a render-unsafe fn that we would use only to init HOC state? Couldn't this lead to users trying to using it directly and being confused when no re-renders happen? Would this function be documented for general usage?

This comment has been minimized.

@brentvatne

brentvatne Feb 27, 2018

Member

i think if they use it in a function and are confused by no re-renders happening then they have misunderstood how react works, that's not really a problem of the library

This comment has been minimized.

@slorber

slorber Feb 27, 2018

Contributor

that's true, and it still can be advised to use the HOC anyway.
I'll try to check soon how to provide that isFocused func

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