Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/Controls/src/Core/NavigationPage/NavigationPageToolbar.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ void UpdateBackButton()

// Once we have better logic inside core to handle backbutton visiblity this
// code should all go away.
// Windows currently doesn't have logic in core to handle back button visibility
// Windows currently doesn't have logic in core to handle back button visibility
// Android just handles it as part of core which means you get cool animations
// that we don't want to interrupt here.
// that we don't want to interrupt here.
// Once it's all built into core we can remove this code and simplify visibility logic
if (_currentPage.IsSet(NavigationPage.HasBackButtonProperty))
{
Expand Down Expand Up @@ -273,7 +273,23 @@ void OnPropertyChanged(object sender, System.ComponentModel.PropertyChangedEvent

Color GetBarTextColor() => _currentNavigationPage?.BarTextColor;
Color GetIconColor() => (_currentPage != null) ? NavigationPage.GetIconColor(_currentPage) : null;
string GetTitle() => GetTitleView() != null ? String.Empty : _currentPage?.Title;

string GetTitle()
{
if (GetTitleView() != null)
{
return string.Empty;
}

Page target = _currentPage;

if (_currentNavigationPage?.CurrentPage is TabbedPage tabbedPage && !string.IsNullOrEmpty(tabbedPage.Title))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this needs to check the page type at all.

I'm testing a few variations of pages on Xamarin.Forms, and the Title always seems to just be whatever the current page is on the navigation page.

image

Is there ever a scenario where
_currentNavigationPage.CurrentPage.Title isn't what we want here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _currentNavigationPage.CurrentPage.Title should be the correct value in all scenarios.

{
target = tabbedPage;
}

return target?.Title;
}

VisualElement GetTitleView()
{
Expand Down
33 changes: 33 additions & 0 deletions src/Controls/tests/Core.UnitTests/ToolbarTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,39 @@ public async Task TitleAndTitleViewAreMutuallyExclusive()
Assert.Equal("Test Title", toolbar.Title);
}

[Fact]
public void ToolbarTitle_UsesTabbedPageChildTitle()
{
var window = new Window();
IToolbarElement toolbarElement = window;
var childPage = new ContentPage { Title = "Child Test Title" };
var tabbedPage = new TabbedPage
{
Children = { childPage },
};
window.Page = new NavigationPage(tabbedPage);

var toolbar = (Toolbar)toolbarElement.Toolbar;
Assert.Equal(childPage.Title, toolbar.Title);
}


[Fact]
public void ToolbarTitle_UsesTabbedPageTitleWhenSet()
{
var window = new Window();
IToolbarElement toolbarElement = window;
var tabbedPage = new TabbedPage
{
Title = "Test Title",
Children = { new ContentPage { Title = "Child Test Title" } },
};
window.Page = new NavigationPage(tabbedPage);

var toolbar = (Toolbar)toolbarElement.Toolbar;
Assert.Equal(tabbedPage.Title, toolbar.Title);
}

[Fact]
public async Task InsertPageBeforeRootPageShowsBackButton()
{
Expand Down
37 changes: 37 additions & 0 deletions src/Controls/tests/DeviceTests/Elements/Toolbar/ToolbarTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,43 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), (handl
});
}

[Fact(DisplayName = "Toolbar Uses TabbedPage Title When Set")]
public async Task ToolbarTabbedPageTitle()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's alright to remove these. We have tests that validate that Toolbar.Title makes it to the platform. So, I think the unit tests plus those tests cover what we need.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, removed and added more tests validating the title changing tabs etc.

{
SetupBuilder();
var navPage = new NavigationPage(
new TabbedPage
{
Title = "Tabbed Page Title",
Children = { new ContentPage { Title = "Child Page Title" } },
});

await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), (handler) =>
{
string title = GetToolbarTitle(handler);
Assert.Equal("Tabbed Page Title", title);
return Task.CompletedTask;
});
}

[Fact(DisplayName = "Toolbar Uses TabbedPage Child Title")]
public async Task ToolbarTabbedPageChildTitle()
{
SetupBuilder();
var navPage = new NavigationPage(
new TabbedPage
{
Children = { new ContentPage { Title = "Child Page Title" } },
});

await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), (handler) =>
{
string title = GetToolbarTitle(handler);
Assert.Equal("Child Page Title", title);
return Task.CompletedTask;
});
}

[Theory]
[InlineData($"{nameof(FlyoutPage)}WithNavigationPage, {nameof(ContentPage)}, {nameof(FlyoutPage)}WithNavigationPage")]
[InlineData($"{nameof(FlyoutPage)}WithNavigationPage, {nameof(FlyoutPage)}, {nameof(FlyoutPage)}WithNavigationPage")]
Expand Down