Skip to content

Commit 3e68b16

Browse files
JeremyCaneyJeremyCaney
committed
Merge branch 'improvement/INavigationTopicViewModel-pruning' into develop
Previously, the `INavigationTopicViewModel` implemented `ITopicViewModel`, which carried with it a lot of properties that most view components won't require. In fact, the view components themselves only require `IHierarchicalTopicViewModel`, in order to evaluate hierarchical navigation. Not only does this make creating one-off `NavigationTopicViewModel` types cumbersome—and especially if they aren't deriving from `TopicViewModel`—but it also introduces extra work for the `IHierarchicalTopicMappingService` that may not ever be used. Given this, two updates have been made. First, the navigation view components have been updated to operate off of `IHierarchicalTopicViewModel` instead, offering more flexibility. Second, the `INavigationTopicViewModel` no longer derives from `ITopicViewModel`, and instead only defines the couple of properties—namely `Title` and `WebPath`—that it is expected _most_ navigation views will require. In practice, we expect this will have minimal impact on implementations since the properties that are no longer references should rarely be used by navigation—but, if they are, implementers can easily add them to their own navigation view models as needed. In addition, instead of mapping `UniqueKey`, we now map `WebPath`, since navigation components are expected to be expose links to web pages. With this, the `IsSelected()` method has been updated to compare a `webPath` parameter, instead of a `uniqueKey` parameter. This is a breaking change. We expect this will be more intuitive to implementers, however, while also offering more flexibility, since now any arbitrary path can be passed by the view for comparison, if appropriate. This might be appropriate, for instance, if the current topic lives outside the scope of the current navigation, but is associated with a page or section within the navigation—as is often the case with forms.
2 parents 219ece4 + 045100b commit 3e68b16

File tree

9 files changed

+115
-54
lines changed

9 files changed

+115
-54
lines changed

‎OnTopic.AspNetCore.Mvc.Host/Views/Shared/Components/Menu/Default.cshtml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
IHtmlContent WriteMenu(NavigationTopicViewModel topic, int indentLevel = 1) => Body(
1818

1919
@<li>
20-
<a href="@topic.WebPath">@(topic.ShortTitle?? topic.Title?? topic.Key)</a>
20+
<a href="@topic.WebPath">@(topic.ShortTitle?? topic.Title)</a>
2121
<ul>
2222
@foreach (var childTopic in topic.Children) {
2323
@WriteMenu(childTopic, indentLevel+1);

‎OnTopic.AspNetCore.Mvc.Tests/TopicViewComponentTest.cs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,10 @@ public async Task Menu_Invoke_ReturnsNavigationViewModel() {
114114
var model = concreteResult?.ViewData.Model as NavigationViewModel<NavigationTopicViewModel>;
115115

116116
Assert.IsNotNull(model);
117-
Assert.AreEqual<string?>(_topic.GetUniqueKey(), model?.CurrentKey);
118-
Assert.AreEqual<string?>("Root:Web", model?.NavigationRoot?.UniqueKey);
117+
Assert.AreEqual<string?>(_topic.GetUniqueKey(), model?.CurrentWebPath);
118+
Assert.AreEqual<string?>("/Web/", model?.NavigationRoot?.WebPath);
119119
Assert.AreEqual<int?>(3, model?.NavigationRoot?.Children.Count);
120-
Assert.IsTrue(model?.NavigationRoot?.IsSelected(_topic.GetUniqueKey())?? false);
120+
Assert.IsTrue(model?.NavigationRoot?.IsSelected(_topic.GetWebPath())?? false);
121121

122122
}
123123

@@ -139,10 +139,10 @@ public async Task PageLevelNavigation_Invoke_ReturnsNavigationViewModel() {
139139
var model = concreteResult?.ViewData.Model as NavigationViewModel<NavigationTopicViewModel>;
140140

141141
Assert.IsNotNull(model);
142-
Assert.AreEqual<string?>(_topic.GetUniqueKey(), model?.CurrentKey);
143-
Assert.AreEqual<string?>("Root:Web:Web_3", model?.NavigationRoot?.UniqueKey);
142+
Assert.AreEqual<string?>(_topic.GetUniqueKey(), model?.CurrentWebPath);
143+
Assert.AreEqual<string?>("/Web/Web_3/", model?.NavigationRoot?.WebPath);
144144
Assert.AreEqual<int?>(2, model?.NavigationRoot?.Children.Count);
145-
Assert.IsTrue(model?.NavigationRoot?.IsSelected(_topic.GetUniqueKey())?? false);
145+
Assert.IsTrue(model?.NavigationRoot?.IsSelected(_topic.GetWebPath())?? false);
146146

147147
}
148148

‎OnTopic.AspNetCore.Mvc/Components/MenuViewComponentBase{T}.cs‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,29 @@ namespace OnTopic.AspNetCore.Mvc.Components {
2323
/// </summary>
2424
/// <remarks>
2525
/// <para>
26-
/// As a best practice, global data required by the layout view are requested independent of the current page. This
27-
/// allows each layout element to be provided with its own layout data, in the form of <see
28-
/// cref="NavigationViewModel{T}"/>s, instead of needing to add this data to every view model returned by <see
29-
/// cref="TopicController"/>.
26+
/// As a best practice, global data required by the layout view are requested independent of the current page. This allows
27+
/// each layout element to be provided with its own layout data, in the form of a <see cref="NavigationViewModel{T}"/>s,
28+
/// instead of needing to add this data to every view model returned by e.g. a <see cref="TopicController"/>.
3029
/// </para>
3130
/// <para>
3231
/// In order to remain view model agnostic, the <see cref="NavigationViewModel{T}"/> does not assume that a particular
3332
/// view model will be used, and instead accepts a generic argument for any view model that implements the interface <see
34-
/// cref="INavigationTopicViewModel{T}"/>. Since generic view components cannot be effectively routed to, however, that
35-
/// means implementors must, at minimum, provide a local instance of <see cref="NavigationViewModel{T}"/> which sets the
33+
/// cref="IHierarchicalTopicViewModel{T}"/>. Since generic view components cannot be effectively routed to, however, that
34+
/// means implementors must, at minimum, provide a local instance of <see cref="NavigationViewModel{T}"/>, which sets the
3635
/// generic value to the desired view model. To help enforce this, while avoiding ambiguity, this class is marked as
3736
/// <c>abstract</c> and suffixed with <c>Base</c>.
3837
/// </para>
38+
/// <para>
39+
/// While the <see cref="MenuViewComponentBase{T}"/> only requires that the <typeparamref name="T"/> implement <see cref="
40+
/// IHierarchicalTopicViewModel{T}"/>, views will require additional properties. These can be determined on a per-case
41+
/// basis, as required by the implementation. Implementaters, however, should consider implementing the <see cref="
42+
/// INavigationTopicViewModel{T}"/> interface, which provides the standard properties that most views will likely need, as
43+
/// well as a <see cref="INavigationTopicViewModel{T}.IsSelected(String)"/> method for determining if the navigation item
44+
/// is currently selected.
45+
/// </para>
3946
/// </remarks>
4047
public abstract class MenuViewComponentBase<T> :
41-
NavigationTopicViewComponentBase<T> where T : class, INavigationTopicViewModel<T>, new()
48+
NavigationTopicViewComponentBase<T> where T : class, IHierarchicalTopicViewModel<T>, new()
4249
{
4350

4451
/*==========================================================================================================================
@@ -124,7 +131,7 @@ public async Task<IViewComponentResult> InvokeAsync() {
124131
\-----------------------------------------------------------------------------------------------------------------------*/
125132
var navigationViewModel = new NavigationViewModel<T>() {
126133
NavigationRoot = await MapNavigationTopicViewModels(navigationRootTopic).ConfigureAwait(true),
127-
CurrentKey = CurrentTopic?.GetUniqueKey()?? HttpContext.Request.Path
134+
CurrentWebPath = CurrentTopic?.GetUniqueKey()?? HttpContext.Request.Path
128135
};
129136

130137
/*------------------------------------------------------------------------------------------------------------------------

‎OnTopic.AspNetCore.Mvc/Components/NavigationTopicViewComponentBase{T}.cs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
| Client Ignia, LLC
44
| Project Topics Library
55
\=============================================================================================================================*/
6+
using System;
67
using Microsoft.AspNetCore.Mvc;
78
using OnTopic.Mapping.Hierarchical;
89
using OnTopic.Models;
@@ -19,10 +20,20 @@ namespace OnTopic.AspNetCore.Mvc.Components {
1920
/// model.
2021
/// </summary>
2122
/// <remarks>
23+
/// <para>
2224
/// This class is intended to provide a foundation for concrete implementations. It is not a fully formed implementation
2325
/// itself. As a result, it is marked as <c>abstract</c>.
26+
/// </para>
27+
/// <para>
28+
/// While the <see cref="NavigationTopicViewComponentBase{T}"/> only requires that the <typeparamref name="T"/> implement
29+
/// <see cref="IHierarchicalTopicViewModel{T}"/>, views will require additional properties. These can be determined on a
30+
/// per-case basis, as required by the implementation. Implementaters, however, should consider implementing the <see cref
31+
/// ="INavigationTopicViewModel{T}"/> interface, which provides the standard properties that most views will likely need,
32+
/// as well as a <see cref="INavigationTopicViewModel{T}.IsSelected(String)"/> method for determining if the navigation
33+
/// item is currently selected.
34+
/// </para>
2435
/// </remarks>
25-
public abstract class NavigationTopicViewComponentBase<T> : ViewComponent where T : class, INavigationTopicViewModel<T>, new() {
36+
public abstract class NavigationTopicViewComponentBase<T> : ViewComponent where T : class, IHierarchicalTopicViewModel<T>, new() {
2637

2738
/*==========================================================================================================================
2839
| PRIVATE VARIABLES

‎OnTopic.AspNetCore.Mvc/Components/PageLevelNavigationViewComponentBase{T}.cs‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
| Client Ignia, LLC
44
| Project Topics Library
55
\=============================================================================================================================*/
6+
using System;
67
using System.Threading.Tasks;
78
using Microsoft.AspNetCore.Mvc;
89
using OnTopic.AspNetCore.Mvc.Controllers;
@@ -30,14 +31,22 @@ namespace OnTopic.AspNetCore.Mvc.Components {
3031
/// <para>
3132
/// In order to remain view model agnostic, the <see cref="PageLevelNavigationViewComponentBase{T}"/> does not assume that
3233
/// a particular view model will be used, and instead accepts a generic argument for any view model that implements the
33-
/// interface <see cref="INavigationTopicViewModel{T}"/>. Since generic view components cannot be effectively routed to,
34+
/// interface <see cref="IHierarchicalTopicViewModel{T}"/>. Since generic view components cannot be effectively routed to,
3435
/// however, that means implementors must, at minimum, provide a local instance of <see
3536
/// cref="PageLevelNavigationViewComponentBase{T}"/> which sets the generic value to the desired view model. To help
3637
/// enforce this, while avoiding ambiguity, this class is marked as <c>abstract</c> and suffixed with <c>Base</c>.
3738
/// </para>
39+
/// <para>
40+
/// While the <see cref="PageLevelNavigationViewComponentBase{T}"/> only requires that the <typeparamref name="T"/>
41+
/// implement <see cref="IHierarchicalTopicViewModel{T}"/>, views will require additional properties. These can be
42+
/// determined on a per-case basis, as required by the implementation. Implementaters, however, should consider
43+
/// implementing the <see cref="INavigationTopicViewModel{T}"/> interface, which provides the standard properties that
44+
/// most views will likely need, as well as a <see cref="INavigationTopicViewModel{T}.IsSelected(String)"/> method for
45+
/// determining if the navigation item is currently selected.
46+
/// </para>
3847
/// </remarks>
3948
public abstract class PageLevelNavigationViewComponentBase<T> :
40-
NavigationTopicViewComponentBase<T> where T : class, INavigationTopicViewModel<T>, new()
49+
NavigationTopicViewComponentBase<T> where T : class, IHierarchicalTopicViewModel<T>, new()
4150
{
4251

4352
/*==========================================================================================================================
@@ -119,7 +128,7 @@ public async Task<IViewComponentResult> InvokeAsync() {
119128
\-----------------------------------------------------------------------------------------------------------------------*/
120129
var navigationViewModel = new NavigationViewModel<T>() {
121130
NavigationRoot = await MapNavigationTopicViewModels(navigationRootTopic).ConfigureAwait(true),
122-
CurrentKey = CurrentTopic?.GetUniqueKey()?? HttpContext.Request.Path
131+
CurrentWebPath = CurrentTopic?.GetUniqueKey()?? HttpContext.Request.Path
123132
};
124133

125134
/*------------------------------------------------------------------------------------------------------------------------

‎OnTopic.AspNetCore.Mvc/Models/NavigationViewModel{T}.cs‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,24 @@ namespace OnTopic.AspNetCore.Mvc.Models {
1313
| VIEW MODEL: NAVIGATION TOPIC
1414
\---------------------------------------------------------------------------------------------------------------------------*/
1515
/// <summary>
16-
/// Provides a strongly-typed data transfer object for feeding views with information about a tier of navigation.
16+
/// Provides a strongly-typed view model for feeding views with information expected to be required for each node in of
17+
/// navigation.
1718
/// </summary>
1819
/// <remarks>
1920
/// <para>
2021
/// No topics are expected to have a <c>Navigation</c> content type. Instead, this view model is expected to be manually
2122
/// constructed by the <see cref="NavigationTopicViewComponentBase{T}"/>.
2223
/// </para>
2324
/// <para>
24-
/// The <see cref="NavigationRoot"/> can be any view model that implements <see cref="INavigationTopicViewModel{T}"/>,
25-
/// which provides a base level of support for properties associated with the typical <c>Page</c> content type as well as
26-
/// a method for determining if a given <see cref="INavigationTopicViewModel{T}"/> instance is the currently-selected
27-
/// topic. Implementations may support additional properties, as appropriate.
25+
/// The <see cref="NavigationRoot"/> can be any view model that implements <see cref="IHierarchicalTopicViewModel{T}"/>,
26+
/// which ensures support for hierarchical coverage. In practice, we expect most implementers will choose to implement
27+
/// <see cref="INavigationTopicViewModel{T}"/> instead, which addresses not only <see cref="IHierarchicalTopicViewModel{T}
28+
/// "/>, but also provides a base level of support for properties that most navigation views will need, as well as a
29+
/// method for determining if a given <see cref="IHierarchicalTopicViewModel{T}"/> instance is the currently-selected
30+
/// topic. Derived implementations may introduce additional properties, as appropriate.
2831
/// </para>
2932
/// </remarks>
30-
public class NavigationViewModel<T> where T : class, INavigationTopicViewModel<T> {
33+
public class NavigationViewModel<T> where T : class, IHierarchicalTopicViewModel<T> {
3134

3235
/*==========================================================================================================================
3336
| NAVIGATION ROOT
@@ -37,32 +40,42 @@ public class NavigationViewModel<T> where T : class, INavigationTopicViewModel<T
3740
/// </summary>
3841
/// <remarks>
3942
/// Since this implements <see cref="IHierarchicalTopicViewModel{T}"/>, it may include multiple levels of children. By
40-
/// implementing it as a generic, each site or application can provide its own <see cref="INavigationTopicViewModel{T}"/>
41-
/// implementation, thus potentially extending the schema with properties relevant to that site's navigation. For example,
42-
/// a site may optionally add an <c>IconUrl</c> property if it wishes to assign unique icons to each link in the
43+
/// implementing it as a generic, each site or application can provide its own <see cref="IHierarchicalTopicViewModel{T}"
44+
/// /> implementation, thus potentially extending the schema with properties relevant to that site's navigation. For
45+
/// example, a site may optionally add an <c>IconUrl</c> property if it wishes to assign unique icons to each link in the
4346
/// navigation.
4447
/// </remarks>
4548
public T? NavigationRoot { get; set; }
4649

4750
/*==========================================================================================================================
48-
| CURRENT KEY
51+
| CURRENT WEB PATH
4952
\-------------------------------------------------------------------------------------------------------------------------*/
5053
/// <summary>
51-
/// The <see cref="Topic.GetUniqueKey()"/> representing the path to the current <see cref="Topic"/>.
54+
/// The <see cref="Topic.GetWebPath()"/> representing the path to the current <see cref="Topic"/>.
5255
/// </summary>
5356
/// <remarks>
5457
/// <para>
5558
/// In order to determine whether any given <see cref="INavigationTopicViewModel{T}.IsSelected(String)"/>, the views
56-
/// will need to know where in the hierarchy the user currently is. By storing this on the <see
57-
/// cref="NavigationViewModel{T}"/> used as the root view model for every navigation component, we ensure that the views
59+
/// will need to know where in the hierarchy the user currently is. By storing this on the <see cref="
60+
/// NavigationViewModel{T}"/> used as the root view model for every navigation component, we ensure that the views
5861
/// always have access to this information.
5962
/// </para>
6063
/// <para>
61-
/// It's worth noting that while this <i>could</i> be stored on the <see cref="INavigationTopicViewModel{T}"/> itself,
64+
/// It's worth noting that while this <i>could</i> be stored on the <see cref="IHierarchicalTopicViewModel{T}"/> itself,
6265
/// that would prevent those values from being cached. As such, it's preferrable to keep the navigation nodes stateless,
6366
/// and maintaining state exclusively in the <see cref="NavigationViewModel{T}"/> itself.
6467
/// </para>
6568
/// </remarks>
69+
public string CurrentWebPath { get; set; } = default!;
70+
71+
/*==========================================================================================================================
72+
| CURRENT KEY
73+
\-------------------------------------------------------------------------------------------------------------------------*/
74+
/// <summary>
75+
/// The <see cref="Topic.GetWebPath()"/> representing the path to the current <see cref="Topic"/>.
76+
/// </summary>
77+
/// <inheritdoc cref="CurrentWebPath"/>
78+
[Obsolete("The CurrentKey property has been replaced in favor of CurrentWebPath.", true)]
6679
public string CurrentKey { get; set; } = default!;
6780

6881
} //Class

‎OnTopic.ViewModels/NavigationTopicViewModel.cs‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,23 @@ namespace OnTopic.ViewModels {
1818
/// <remarks>
1919
/// <para>
2020
/// No topics are expected to have a <c>Navigation</c> content type. Instead, this view model is expected to be manually
21-
/// constructed by e.g. a <c>LayoutController</c>.
21+
/// constructed by e.g. a <c>MenuViewComponent</c>.
2222
/// </para>
2323
/// <para>
24-
/// Since C# doesn't support return-type covariance, this class can't be derived in a meaningful way (i.e., if it were to
25-
/// be, the <see cref="NavigationTopicViewModel.Children"/> property would still return a <see cref="Collection{T}"/> of
26-
/// <see cref="NavigationTopicViewModel"/> instances). Instead, the preferred way to extend the functionality is to create
27-
/// a new implementation of <see cref="INavigationTopicViewModel{T}"/>. To help communicate this, the <see
24+
/// Since .NET Standard doesn't support return-type covariance, this class can't be derived in a meaningful way (i.e., if
25+
/// it were to be, the <see cref="NavigationTopicViewModel.Children"/> property would still return a <see cref="Collection
26+
/// {T}"/> of <see cref="NavigationTopicViewModel"/> instances). Instead, the preferred way to extend the functionality is
27+
/// to create a new implementation of <see cref="INavigationTopicViewModel{T}"/>. To help communicate this, the <see
2828
/// cref="NavigationTopicViewModel"/> class is marked as <c>sealed</c>.
2929
/// </para>
3030
/// </remarks>
31-
public sealed record NavigationTopicViewModel : TopicViewModel, INavigationTopicViewModel<NavigationTopicViewModel> {
31+
public sealed record NavigationTopicViewModel : INavigationTopicViewModel<NavigationTopicViewModel> {
32+
33+
/*==========================================================================================================================
34+
| TITLE
35+
\-------------------------------------------------------------------------------------------------------------------------*/
36+
/// <inheritdoc cref="TopicViewModel"/>
37+
public string? Title { get; init; }
3238

3339
/*==========================================================================================================================
3440
| SHORT TITLE
@@ -38,6 +44,12 @@ public sealed record NavigationTopicViewModel : TopicViewModel, INavigationTopic
3844
/// </summary>
3945
public string? ShortTitle { get; init; }
4046

47+
/*==========================================================================================================================
48+
| WEB PATH
49+
\-------------------------------------------------------------------------------------------------------------------------*/
50+
/// <inheritdoc cref="WebPath"/>
51+
public string? WebPath { get; init; }
52+
4153
/*==========================================================================================================================
4254
| CHILDREN
4355
\-------------------------------------------------------------------------------------------------------------------------*/
@@ -53,8 +65,8 @@ public sealed record NavigationTopicViewModel : TopicViewModel, INavigationTopic
5365
/// Determines whether or not the node represented by this <see cref="NavigationTopicViewModel"/> is currently selected,
5466
/// typically meaning the user is on the page this object is pointing to.
5567
/// </summary>
56-
public bool IsSelected(string uniqueKey) =>
57-
$"{uniqueKey}:".StartsWith($"{UniqueKey}:", StringComparison.OrdinalIgnoreCase);
68+
public bool IsSelected(string webPath) =>
69+
$"{webPath}/".StartsWith($"{WebPath}", StringComparison.OrdinalIgnoreCase);
5870

5971
} //Class
6072
} //Namespace

0 commit comments

Comments
 (0)