dewango / BottomNavigationBarXF

Bottom Navigation Bar for Xamarin Forms
MIT License
187 stars 97 forks source link

Added Fix as proposed in #6 and added Fixed Mode and Textcolor #16

Closed escamoteur closed 7 years ago

escamoteur commented 7 years ago

6 Added the proposed fix

escamoteur commented 7 years ago

Added a property FixedMode to BottombarPage so that you now can also use FixedMode.

When selecting FixedMode also the TabText and Icon color can be set by setting BarTextColor

You can only select a the Text and Icon Color of the active Tab when using Fixed Mode

andreinitescu commented 7 years ago

Thank you for your contribution! Can you please remove the changes which are not really part of the PR? Please just keep the actual code, not changes due to opening solution in a different IDE. Just look to the changes of BottomBar.Droid/BottomBar.Droid.csproj and BottomBarXF.sln to see what I mean. There's also some code indentation\formatting issues... Thanks!

escamoteur commented 7 years ago

This is strange, when using FixedMode I can set the TextColor, but no longer the Bar Color. any idea?

escamoteur commented 7 years ago

Ok, found out how it works. You have to set DarkTheme and then provide a colors.xml file. I added a property for the theming to BottomBarPage

escamoteur commented 7 years ago

Found today that the CurrentPageChangedEvent was called twice because it was called due to the asignment of the new Page to CurrentPage which triggers an OnPropertyChanged event and on the following call to RaiseCurrentPageChanged.

I changed it to

        public void OnTabSelected (int position)
        {
            //Do we need this call? It's also done in OnElementPropertyChanged
            SwitchContent(Element.Children [position]);
            var bottomBarPage = Element as BottomBarPage;
            bottomBarPage.CurrentPage = Element.Children[position];
            //bottomBarPage.RaiseCurrentPageChanged();
        }

As I wrote in the comment I'm not sure if we need the first call to SwitchContent as it get called again when the CurrentPage property get's changed:

        protected override void OnElementPropertyChanged (object sender, PropertyChangedEventArgs e)
        {
            base.OnElementPropertyChanged (sender, e);

            if (e.PropertyName == nameof (TabbedPage.CurrentPage)) {
                SwitchContent (Element.CurrentPage);
            } else if (e.PropertyName == NavigationPage.BarBackgroundColorProperty.PropertyName) {
                UpdateBarBackgroundColor ();
            } else if (e.PropertyName == NavigationPage.BarTextColorProperty.PropertyName) {
                UpdateBarTextColor ();
            }
        }

If you comment it out it still works.

escamoteur commented 7 years ago

@sschmidt did you have a loom at my PR?

nbsoftware commented 7 years ago

Hello @escamoteur ! Thanks for your contributions ;) Actually you are right, I also just found out about the CurrentPageChangedEvent being called twice. So your fix is correct, I did the same and I actually also removed the line that calls SwitchContent in OnTabSelected because it is indeed done in OnElementPropertyChanged. So the function looks like that:

        public void OnTabSelected (int position)
        {
            var bottomBarPage = Element as BottomBarPage;
            // setting the CurrentPage on XF control will raise the corresponding OnElementPropertyChanged call
            // so no need to call SwitchContent from here
            bottomBarPage.CurrentPage = Element.Children[position];
        }

While I am at it I made another fix in SwitchContent for the following scenario: if the CurrentPage property was set programmatically in the PCL code, the correct tab view was indeed updated and shown, but not the corresponding button. Fixed by calling SelectTabAtPosition on native control.


    protected virtual void SwitchContent (Page view)
        {
            // when CurrentPage has been changed programmatically from Xamarin Form control,
            // we need to set the current active tab also on the native control
            _bottomBar.SelectTabAtPosition(Element.Children.IndexOf(Element.CurrentPage), false);

            Context.HideKeyboard (this);

            _frameLayout.RemoveAllViews ();

            if (view == null) {
                return;
            }

            if (Platform.GetRenderer (view) == null) {
                Platform.SetRenderer (view, Platform.CreateRenderer (view));
            }

            _frameLayout.AddView (Platform.GetRenderer (view).ViewGroup);
        }

If you want I can commit those changes to my master branch and send you the PR... Or, as they are really simple, you could apply those 2 changes to yours directly.

Cheers

escamoteur commented 7 years ago

Great, what about the other extensions? what do you think about them?

sschmidt commented 7 years ago

Hi @escamoteur, thanks for the great contribution. Highly appreciated!

Do we really need the changes to the sln and csproj files (i.e. changes to visual studio version/target framework version)? In case those changes don't add to the solution, I'd like to leave those files as they are.

As I understand @nbsoftware and @andreinitescu already approved/tested this PR (thanks guys!). I'll test it tomorrow and we can go ahead and merge this in.

escamoteur commented 7 years ago

Sorry, this seems to be an effect of VS 2015 with Xamarin 4.2. It automatcially changes it when you first open an solution with it. You don't need to merge them in.

escamoteur commented 7 years ago

Great, thanks. Do you built a new nuget?

sschmidt commented 7 years ago

Sure, can you briefly test the master version before I update nuget please?

nbsoftware commented 7 years ago

Hello,

Before releasing to nuget, could you please integrate the two small fixes I mentioned in https://github.com/thrive-now/BottomNavigationBarXF/pull/16#issuecomment-254543261 ?

Would you prefer I issue a PR from current master?

Thanks

sschmidt commented 7 years ago

@nbsoftware sure. A PR would be more than welcome!

escamoteur commented 7 years ago

Did anybody of you try to change the fond/fontsize of the Tabb Texts in fixed mode? One of my texts get's truncated while perfectly visible on iOS.

escamoteur commented 7 years ago

Just checked the current version works fine.