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

Open generic pages and double tabs navigation #1943

Merged
merged 3 commits into from Nov 11, 2019

Conversation

JeremyBP
Copy link
Contributor

@JeremyBP JeremyBP commented Nov 4, 2019

Description of Change

Sometimes, we need to define some generic abstract classes. This could be the case for ContentPage like:

  MyAbstractContentPage<TOtherClass> : ContentPage where TOtherClass : IWhateverInterface

and then:

  MyContentPage : MyAbstractContentPage<MyOtherClass>

The problem is using AutoRegisterForNavigation attribute try to register MyAbstractContentPage and fails with container exception about registering open generic.
So I just added some filters on registration to ignore open generic classes.

Bugs Fixed

  • AutoRegisterForNavigation attribute throw a container registration exception when using generic page class in project

API Changes

Changed:

  • var viewTypes = assembly.ExportedTypes.Where(t => t.IsSubclassOf(typeof(Page)));
    =>
    var viewTypes = assembly.ExportedTypes.Where(t => t.IsSubclassOf(typeof(Page)) && !t.GetTypeInfo().IsGenericTypeDefinition && !t.GetTypeInfo().ContainsGenericParameters);

Behavioral Changes

Unchanged

PR Checklist

  • Has tests
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard

@dnfclas
Copy link

dnfclas commented Nov 4, 2019

CLA assistant check
All CLA requirements met.

@JeremyBP JeremyBP changed the title Ignore open generic pages on auto registration process Open generic pages and double tabs navigation Nov 4, 2019
@JeremyBP
Copy link
Contributor Author

JeremyBP commented Nov 4, 2019

Description of Change

In some scenarios, we need to implement a bottom tab bar (with a standard TabbedPage) and a top tab bar (i.e. with a Naxam TabbedPage) like Facebook does. In this case, we should look at the third parent to get a NavigationPage and beeing able to navigate as expected.

Bugs Fixed

  • Navigating from a ContentPage contained in a top tabbed page itself contained in a bottom tabbed page (double tabs) shows in a modal way as it can't find a NavigationPage in contentPage.Parent?.Parent.

API Changes

Changed:

  • if ((page.Parent is TabbedPage || page.Parent is CarouselPage) && page.Parent?.Parent is NavigationPage navigationParent)
    =>
    if ((page.Parent is TabbedPage || page.Parent is CarouselPage) && (page.Parent?.Parent is TabbedPage ? page.Parent?.Parent?.Parent : page.Parent?.Parent) is NavigationPage navigationParent)

Behavioral Changes

Unchanged

PR Checklist

  • Has tests
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard

@dansiegel
Copy link
Member

@JeremyBP in the future you really need to open an issue so we can discuss this first. I agree that the issue of Open Generics is a use case that was not considered when Auto Registration was added so I have no problem adding that piece. But I need to think a bit on your solution for the TabbedPage issue... if you can do an interactive rebase to remove the second commit I am happy to merge the first piece.

@JeremyBP
Copy link
Contributor Author

JeremyBP commented Nov 5, 2019

As I'm not "rebase aware" so far, I reverted last commit.
I know this is not the rebase you expected (and I should learn about it), so another option would be to simply reject this PR and update your code manualy with open generic ignore filters.
No authorship ambition on it ;)
About the double tab bar, I'll open an issue.

@dansiegel dansiegel merged commit 90828a1 into PrismLibrary:master Nov 11, 2019
@lock
Copy link

lock bot commented Jan 28, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants