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

escamoteur · 2016-10-11T18:07:39Z

#6 Added the proposed fix

Space above bottom navigation bar dewango#6

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

escamoteur · 2016-10-12T11:37:06Z

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 · 2016-10-12T11:45:52Z

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!

andreinitescu · 2016-10-12T11:47:34Z

BottomBar.XamarinForms/BottomBarPage.cs

@@ -21,7 +21,9 @@ namespace BottomBar.XamarinForms
 {
 	public class BottomBarPage : TabbedPage
 	{
-		public void RaiseCurrentPageChanged()
+        public bool FixedMode { get; set; }


code is not formatted....

andreinitescu · 2016-10-12T11:48:10Z

BottomBar.Droid/Renderers/BottomBarPageRenderer.cs

@@ -132,7 +132,11 @@ protected override void OnElementChanged (ElementChangedEventArgs<BottomBarPage>
 					// create bottomBar control
 					_bottomBar = BottomNavigationBar.BottomBar.Attach (_frameLayout, null);
 					_bottomBar.NoTabletGoodness ();
-					_bottomBar.LayoutParameters = new LayoutParams (LayoutParams.MatchParent, LayoutParams.MatchParent);
+				    if (bottomBarPage.FixedMode)


Could you please format the code?

andreinitescu · 2016-10-12T11:48:24Z

BottomBar.Droid/Renderers/BottomBarPageRenderer.cs

@@ -227,7 +231,8 @@ void UpdateBarTextColor ()
 				return;
 			}

-			// haven't found yet how to set text color for tab items on_bottomBar, doesn't seem to have a direct way
+            _bottomBar.SetActiveTabColor(Element.BarTextColor.ToAndroid());


same here...

Strange it's formated at my side.

Doesn't appear so: https://github.com/escamoteur/BottomNavigationBarXF/blob/b07806b2d7bf57fd348e12720ea3d39f5a97fcb9/BottomBar.Droid/Renderers/BottomBarPageRenderer.cs#L234

Ah, I see, seems a problem with tabs vs / spaces. had to switch to tabs yesterday. will change it.

Should I create a new branch and make a new PR or what is the best way?

Also I have no idea why my VS is changing the project files. I did not change anything there

That's strange. must be another problem.
I create a new branch

This is my first PR to this project and I felt you comments quite harsh as if I'm an idiot who cannot format his code before pushing.

I'm sorry if you feel that way. Please read again, I didn't have any harsh comments, I just asked if you could remove the unnecessary changes, which add a lot of noise to the PR. That's in order to maintain a clean code as possible.

Don't want to push this further. Perhaps the next time first welcome a new contributor. It's about motivation to contribute on a project.
I'm fine now, we found the reason.

I didn't welcome you? I'm sorry for that. Welcome again!

Ahh written communication is so difficult if it comes to emotions :-(

escamoteur · 2016-10-12T12:25:32Z

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

escamoteur · 2016-10-12T13:39:12Z

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

… restored

escamoteur · 2016-10-13T14:55:33Z

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 · 2016-10-14T19:58:24Z

@sschmidt did you have a loom at my PR?

nbsoftware · 2016-10-18T15:25:27Z

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 · 2016-10-18T15:27:25Z

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

sschmidt · 2016-10-18T15:50:50Z

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 · 2016-10-18T15:52:27Z

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 · 2016-10-23T16:07:12Z

Great, thanks. Do you built a new nuget?

sschmidt · 2016-10-23T17:33:58Z

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

nbsoftware · 2016-10-24T06:57:24Z

Hello,

Before releasing to nuget, could you please integrate the two small fixes I mentioned in #16 (comment) ?

Would you prefer I issue a PR from current master?

Thanks

sschmidt · 2016-10-24T07:10:39Z

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

escamoteur · 2016-10-24T08:12:10Z

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 · 2016-10-24T12:13:13Z

Just checked the current version works fine.

escamoteur added 2 commits October 11, 2016 20:04

Added Fix as proposed in

9cbce1c

Space above bottom navigation bar dewango#6

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

b07806b

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

escamoteur changed the title ~~Added Fix as proposed in #6~~ Added Fix as proposed in #6 and added Fixed Mode and Textcolor Oct 12, 2016

escamoteur mentioned this pull request Oct 12, 2016

is there a way to hide pink bar just above bottom toolbar in the sample? #13

Open

andreinitescu reviewed Oct 12, 2016

View reviewed changes

escamoteur added 3 commits October 12, 2016 15:41

Added Property for setting the Theme of FixedMode BottomBars

64dd062

Commented out the new settings so that the original look and feel is…

0ba50bf

… restored

Bugfix in CustomRenderer CurentPageChanged event was called twice

af7cf9c

sschmidt merged commit af7cf9c into dewango:master Oct 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

escamoteur commented Oct 11, 2016

escamoteur commented Oct 12, 2016

andreinitescu commented Oct 12, 2016 •

edited

Loading

andreinitescu Oct 12, 2016

andreinitescu Oct 12, 2016

andreinitescu Oct 12, 2016

escamoteur Oct 12, 2016

andreinitescu Oct 12, 2016

escamoteur Oct 12, 2016

escamoteur Oct 12, 2016

escamoteur Oct 12, 2016

andreinitescu Oct 12, 2016 •

edited

Loading

escamoteur Oct 12, 2016

andreinitescu Oct 12, 2016

escamoteur Oct 12, 2016

escamoteur commented Oct 12, 2016

escamoteur commented Oct 12, 2016

escamoteur commented Oct 13, 2016

escamoteur commented Oct 14, 2016

nbsoftware commented Oct 18, 2016

escamoteur commented Oct 18, 2016

sschmidt commented Oct 18, 2016

escamoteur commented Oct 18, 2016 •

edited

Loading

escamoteur commented Oct 23, 2016

sschmidt commented Oct 23, 2016

nbsoftware commented Oct 24, 2016

sschmidt commented Oct 24, 2016

escamoteur commented Oct 24, 2016

escamoteur commented Oct 24, 2016

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

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

Conversation

escamoteur commented Oct 11, 2016

escamoteur commented Oct 12, 2016

andreinitescu commented Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreinitescu Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escamoteur commented Oct 12, 2016

escamoteur commented Oct 12, 2016

escamoteur commented Oct 13, 2016

escamoteur commented Oct 14, 2016

nbsoftware commented Oct 18, 2016

escamoteur commented Oct 18, 2016

sschmidt commented Oct 18, 2016

escamoteur commented Oct 18, 2016 • edited Loading

escamoteur commented Oct 23, 2016

sschmidt commented Oct 23, 2016

nbsoftware commented Oct 24, 2016

sschmidt commented Oct 24, 2016

escamoteur commented Oct 24, 2016

escamoteur commented Oct 24, 2016

andreinitescu commented Oct 12, 2016 •

edited

Loading

andreinitescu Oct 12, 2016 •

edited

Loading

escamoteur commented Oct 18, 2016 •

edited

Loading