vectorlit / UnofficialGenconMobile

Code for Unofficial Gen Con Mobile application - Xamarin version.
GNU General Public License v3.0
3 stars 3 forks source link

FEATURE-9 updated code to do events as the app is being used. attemp… #13

Closed garkenxian closed 5 years ago

garkenxian commented 5 years ago

…ted to clean up code base

vectorlit commented 5 years ago

OK I finally had a little time to go through this - let's make a few changes before merge - these changes are to both actually show a loader and to also make future room for a custom notification method:

1) "GenConBusiness" is probably best considered a "view", so you may want to move it to the views folder. 2) We don't currently use Dependency Injection anywhere else in the app - at the moment, all app state is centralized in the GlobalVars class. We could interface that and use it as a DI component on the Views, but I wouldn't suggest going down that route until this pull request is completed. My current suggestion - for the sake of consistency and to make a later DI conversion easier - removing the IGenConBusiness interface and just implementing it as a class, same as the other "Gen****" views for now. I think DI requires a further discussion as it would make it much harder to provide a centralized live-update model for in-app variables. It at least deserves its own issue. 3) Give GenConBusiness.cs a view (see below for pseudo-implementation), leave the other methods in-place. The loader view can be copied from the old App.xaml.cs main page "ShowLoader" method: https://github.com/vectorlit/UnofficialGenconMobile/blob/5ceaa372658b502494d8ce931b05651096873f4f/ConventionMobile/ConventionMobile/App.xaml.cs#L110 It's just 2 labels and a progress bar that updates when the events fire. The class can then make itself invisible when the final load is completed (View.isVisible = false) 4) Remove _business argument in GenMapPage and GenSearchPage and point them at GlobalVars.LoadingState instead 5) Create a new holder for the main page called GenHomePageHolder. (see below for implementation) 6) Change App.xaml.cs's "HomePage" to be the new GenHomePageHolder from 5 above

So the layout for this might be: GlobalVars.cs:

// ... existing code
public static LoadingState loadingState { get; set; } // moved from service

GenConBusiness.cs (look at GenEventCell.cs for a starter example if you have trouble understanding code below):

public class GenConBusiness : ViewCell {
 // layoutDefinition (define loading view here - use the same view as the original loading screen)
 // 
 // View = layoutDefinition

 // ... existing Task methods, which are not dependent on the page being a view or not
}

GenHomePageHolder.cs (this page could later be used for further custom in-app notifications):

public GenHomePageHolder()
{
    Content = new StackLayout
    {
        HorizontalOptions = LayoutOptions.FillAndExpand,
        VerticalOptions = LayoutOptions.FillAndExpand,
        Orientation = StackOrientation.Vertical,
        Margin = 0,
        Padding = 0,

        Children = {
            new GenHomeTabPage(),
            new GenConBusiness()
            // if you have troubles with the view order, try swapping the two above lines (reorder them)
        }
    };
}

App.xaml.cs:

 // ... existing code
public void ShowMainPage()
{
    Device.BeginInvokeOnMainThread(() =>
    {
        HomePage = new GenHomePageHolder();
        MainPage = new NavigationPage(HomePage);
    });
}
 // ... existing code
garkenxian commented 5 years ago
public GenHomePageHolder()
{
    Content = new StackLayout
    {
        HorizontalOptions = LayoutOptions.FillAndExpand,
        VerticalOptions = LayoutOptions.FillAndExpand,
        Orientation = StackOrientation.Vertical,
        Margin = 0,
        Padding = 0,

        Children = {
            new GenHomeTabPage(),
            new GenConBusiness()
            // if you have troubles with the view order, try swapping the two above lines (reorder them)
        }
    };
}

this particular code appeared troublesome, The reason is that the GetHomeTabpage is a Page, not a view, and as such cannot be used within another page. I believe I attempted this already only to have it not work. there are also numerous threads online that are for people attempting to work around this problem. Thus the majority of the reason I ended up using toasts.

As far as the coding style is concerned I will make the requested changes as best I can. Removing the DI and moving some of the business logic to the GlobalVars class, etc

garkenxian commented 5 years ago

image

Error   CS1503  Argument 1: cannot convert from 'ConventionMobile.Views.GenHomeTabPage' to 'Xamarin.Forms.View' ConventionMobile    C:\dev\git\UnofficialGenconMobile\ConventionMobile\ConventionMobile\Views\GenHomePageHolder.cs  23  Active

As Expected the tabbed page does not work in another page. However, I am working on a solution in another branch that would resolve that issue. By converting the tabbedPage to use a tabbed view plugin that would allow us to use the tabbed features but also have a footer (or header) as needed

vectorlit commented 5 years ago

Good call!! I didn't realize when looking at it that the tabbed page would prevent this from working (didn't sink in). The plugin sounds like a chance to workaround; if not - we can figure something else out, just let me know