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

Added property to indicate that the view is inside a Tabbar #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AidenMontgomery
Copy link

I found that the tracking view was floating quite far away from the keyboard when my view was inside a tabbed view controller.
I added a property viewIsInsideTabBar to the view to indicate that the tracking view is inside a tabbar.

@artald
Copy link
Collaborator

artald commented May 23, 2017

Hey @AidenMontgomery, thanks a lot for the PR!

You're right, we're aware of this issue and for now we went with the assumption that it is used in a screen without a tab-bar.

If we're going to make this change I think we should do it a bit different. Instead of using a props and a hard-coded value we can use the bottomLayoutGuide of the current UIViewController. If there's no tab-bar it is supposed to be 0, if there is one it will have the correct height.

Can you make this change? If so, please keep in mind that we need to make sure that it doesn't break anything in current features like the various scroll behaviors (regardless if we're using hard-coded values or not).

Thoughts?

@AidenMontgomery
Copy link
Author

Hi @artald
What you say makes a lot of sense. I will have a look and see if I can make the changes that you have outlined.

@artald
Copy link
Collaborator

artald commented May 23, 2017

@AidenMontgomery thanks, let me know if you have any questions

@AidenMontgomery
Copy link
Author

@artald I am struggling with this a little.

I am attempting to use the following code to get the to current UIViewController

- (UIViewController *)reactViewController:(UIView *)view
{
  id responder = [view nextResponder];
  while (responder) {
    if ([responder isKindOfClass:[UIViewController class]]) {
      return responder;
    }
    responder = [responder nextResponder];
  }
  return nil;
}

This appears to work, in that it responds with a UIViewController. However that UIViewController always has a bottomLayoutGuide.length of 0.

I altered the code a little to print out the type of the responder and the bottomLayoutGuide.length of every responder up the chain, rather than stopping at the first UIViewController.

2017-05-23... <RCCViewController: 0x7fade85643a0> : 0.000000
2017-05-23... <RCCNavigationController: 0x7fadeb821e00> : 49.000000
2017-05-23... <RCCTabBarController: 0x7fade8429970> : 0.000000
2017-05-23... <UIInputWindowController: 0x7fadea064600> : 0.000000
2017-05-23... <UIInputWindowController: 0x7fadea064600> : 0.000000

As you can see, the RCCNavigationController is reporting the height of the tabs.
I was thinking of making the function walk up to the top and returning the first value greater than 0 or eventually 0.

What would be acceptable for this project?

@artald
Copy link
Collaborator

artald commented May 23, 2017

Thanks for all the efforts @AidenMontgomery !

Actually there is a method called reactViewController on a react view (from UIView+React) that we can use to get the view controller, so maybe it is not necessary to add another method, but it's weird that the first responder height is 0 and you need to continue looking for it.

It would be acceptable of course if there's no other way, but I'd like to investigate it a bit further before we push this kind of change. I hope it's not holding you back, I'll need a few days before I can investigate it.

@AidenMontgomery
Copy link
Author

I came across the reactViewController method in some of my research and saw that it did almost the same thing as what I had implemented.
It is not holding me back, as I am now referencing my fork hopefully temporarily. I might put together another example tabbed app that uses react-native-navigation. There also seems to be a small issue with the scroll bar insets, which would be good to show an example of.

@artald
Copy link
Collaborator

artald commented May 23, 2017

Yes, it does almost the same thing except what you added for checking the bottomLayoutGuide
height.

A fork is an excellent temporary solution, I hope we can get this in soon. Also, an example screen with tabs is a great idea. thanks!

@AidenMontgomery
Copy link
Author

AidenMontgomery commented May 23, 2017

I have created a sample application, which somewhat replicates what I am trying to achieve in my project. It is very rough, but demonstrates the issue I was facing.

React Native Keyboard Tracking View Example

I don't know if I have done something wrong in my implementation, but it seemed like the react-native-keyboard-tracking-view only applied to the second tab it was registered in.
If I comment out the control in the second tab, it starts working in the first tab again.

@artald
Copy link
Collaborator

artald commented May 24, 2017

@AidenMontgomery thanks for setting up a demo project!

I didn't understand something - is this issue related to the PR and was caused as a result of the changes to support the tab-bar, or is it a totally separate issue that was occurring anyway?

@AidenMontgomery
Copy link
Author

AidenMontgomery commented May 24, 2017

Apologies for causing confusion, the example application is using the currently published of this module from npm. I was hoping to put together a sample that showed the difference between the behaviour in a tab-bar and without the tab-bar.

I have set up 2 tabs in the application, the first keeps the tabs on screen and was intended to replicate the issue that I originally saw. The second tab hides the tabs and I was hoping that this would then not display the original issue.

However, upon creating the example I have found the issue with the keyboard tracking only applying to the last instance.

@artald
Copy link
Collaborator

artald commented May 24, 2017

Got it. thanks.

I don't have an idea why it only works for the latest instance, we use it multiple times without any issues, but maybe this specific structure of the example is causing this. It's really weird since each view controller has its own root view and they are not related.

Maybe it's best right now to solve one issue at a time. For now the demo app can have just one screen with tabs so we can focus on the bottomLayoutGuide solution.
If necessary we can open a separate issue for the problems you found in the example project when you have 2 screens.

@AidenMontgomery
Copy link
Author

I have set the master branch of the example to reflect the original issue that I created the PR for.
Now there is a second branch first_screen_broken, which shows a potential second issue, where only the second screen works.

I only use this control in one of the tabs in my app, so this second issue is not something that I have seen before and it is not causing me any immediate issues.

@artald
Copy link
Collaborator

artald commented May 24, 2017

Great, thanks. hopefully I'll have time to look into it sometime this week or next week.
In the mean time you can still use your fork, so that's supposed to be good enough for now..

@tafty tafty force-pushed the master branch 2 times, most recently from 8826001 to 87d89c9 Compare January 30, 2019 11:55
@tafty
Copy link

tafty commented Jan 30, 2019

Hi @artald

@AidenMontgomery and I have returned to work for our client where we first experienced the issue of the gap between the keyboard tracking view and the keyboard when inside a tab-bar. Whilst we've been away the reference to Aiden's fork was removed when our client performed a package update. I've just spent some time updating PR #6 to the very latest version of the react-native-keyboard-tracking-view and have amended the PR to a single commit so it's hopefully very clear the changes that have been made. We'd be very grateful if you could take the time to look at PR #6 as, whilst we've once again set our client's project to reference Aiden's fork, there's no guarantee that our client won't revert that change in the future.

Many thanks,

Gareth.

@guyca guyca self-assigned this Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants