ReferringSitesPage Clean C# Markup + DynamicResource Helper

[VincentH-Net]

ReferringSitesPage Clean C# Markup

Added RelativeLayout helpers for this.
Refactored page markup to clean, declarative markup style; moved page UI logic to partial class in separate .logic.cs file.

The full markup now reads like a story, top-down:

This page was tested on iOS and Android, in light and dark mode. Compared screenshots before and after and found no differences.

@brminnick Please discuss if there are elements of this style you personally would prefer not to adopt in GitTrends; I can take it down a notch as needed to respect your preferences. Once this PR is merged, I could do similar PR's for other pages and views.

DynamicResource Helper

In all places where .SetDynamicResource() was used, refactored to .DynamicResource(); also joined multiple this. statements to a single fluent this. statement wherever .DynamicResource() is used.

[VincentH-Net]

The unit test failure seems unrelated to PR changes; it fails with the same error 17 times:

Refit.ApiException : Response status code does not indicate success: 401 (Unauthorized).

[davidortinau]

I have a comment about the legibility of this style more broadly.

After spending some time with this I realized I’m not struggling with RelativeLayout issue. I couldn’t figure out just from reading on my iPad where the CollectionView was, and then I realized it was covered up by the RefreshView. The problem was for me, the RefreshView func didn’t make it clear it was more than a RefreshView.

I acknowledge that part of my issue here is that I’ve been knee deep in Kotlin, Swift, and Dart lately. Had I not been, then my C# eyes would have more clearly picked up on what was written.

So the concern I have, purely about the practice of using functions to return composed controls, is naming. If this would have been RefreshCollectionView instead of RefreshView, then it would have been clear this was a composition.

Is there a better practice around naming that can make this clear from the outset and add legibility/clarity?

RefreshCollectionView is description, but long. Should the name be literal or more contextual like ReferrersList which, given this isn’t the name of a control in Xamarin.Forms should immediately make it clear it’s some composed thing, and I could look and see “oh, it’s a RefreshView wrapping a CollectionView.”

[VincentH-Net]

So the concern I have, purely about the practice of using functions to return composed controls, is naming. If this would have been RefreshCollectionView instead of RefreshView, then it would have been clear this was a composition.

Is there a better practice around naming that can make this clear from the outset and add legibility/clarity?

RefreshCollectionView is description, but long. Should the name be literal or more contextual like ReferrersList which, given this isn’t the name of a control in Xamarin.Forms should immediately make it clear it’s some composed thing, and I could look and see “oh, it’s a RefreshView wrapping a CollectionView.”

Thanks @davidortinau, you confirmed my doubts on the one compromise I made in this refactoring :-)

My initial thought was to rename CollectionView to ReferringSites.
But then I saw that CollectionView was bound to MobileReferringSitesList, and I assumed that the original author deemed this the CollectionView of the page, since the page name is ReferringSitesPage. Since this is a markup style refactoring, I tried to leave the naming intact so the original author could more easily validate equivalence. So I left it unchanged. You cought it and you are 100% right!

In my experience, names should be application/design domain terms, not framework terms. Also, using framework terms as a suffix is best avoided.
Do name composite markup: Header, MobileReferringSites, Title
Don't name composite markup: HeaderGrid, CollectionView, TitleLabel

[brminnick]

Thanks Vincent!

I added some more extension methods for RelativeLayout

public static RelativeLayout Add<TView>(this RelativeLayout relativeLayout, TView view, Bounds bounds) where TView : View?
{
    if (view != null)
        relativeLayout.Children.Add(view, bounds);

    return relativeLayout;
}

public static RelativeLayout Add<TView>(this RelativeLayout relativeLayout, TView view, Expression? x = null, Expression? y = null, Expression? width = null, Expression? height = null) where TView : View?
{
    if (view != null)
        relativeLayout.Children.Add(view, x, y, width, height);

    return relativeLayout;
}

public static RelativeLayout Add<TView>(this RelativeLayout relativeLayout, TView view, Constraint? xConstraint = null, Constraint? yConstraint = null, Constraint? widthConstraint = null, Constraint? heightConstraint = null) where TView : View?
{
    if(view != null)
        relativeLayout.Children.Add(view, xConstraint, yConstraint, widthConstraint, heightConstraint);

    return relativeLayout;
}

[brminnick]

And refactored ReferringSitesPage a bit to use the above methods

public ReferringSitesPage(DeepLinkingService deepLinkingService,
                            ReferringSitesViewModel referringSitesViewModel,
                            Repository repository,
                            IAnalyticsService analyticsService,
                            ThemeService themeService,
                            ReviewService reviewService,
                            IMainThread mainThread) : base(referringSitesViewModel, analyticsService, mainThread)
{
    Title = PageTitles.ReferringSitesPage;

    _repository = repository;
    _themeService = themeService;
    _reviewService = reviewService;
    _deepLinkingService = deepLinkingService;

    reviewService.ReviewCompleted += HandleReviewCompleted;
    ViewModel.PullToRefreshFailed += HandlePullToRefreshFailed;

    var titleRowHeight = _isiOS ? 50 : 0;

    var collectionView = new ReferringSitesCollectionView()
        .Bind(IsVisibleProperty, nameof(ReferringSitesViewModel.IsEmptyDataViewEnabled))
        .Bind(EmptyDataView.TitleProperty, nameof(ReferringSitesViewModel.EmptyDataViewTitle))
        .Bind(EmptyDataView.DescriptionProperty, nameof(ReferringSitesViewModel.EmptyDataViewDescription))
        .Bind(CollectionView.ItemsSourceProperty, nameof(ReferringSitesViewModel.MobileReferringSitesList))
        .Invoke(collectionView => collectionView.SelectionChanged += HandleCollectionViewSelectionChanged);

    var closeButton = new CloseButton(titleRowHeight).Invoke(closeButton => closeButton.Clicked += HandleCloseButtonClicked);

    Content = new RelativeLayout()
                .Add(new ReferringSitesRefreshView(collectionView, repository, _refreshViewCancelltionTokenSource.Token).Assign(out _refreshView)
                        .DynamicResource(RefreshView.RefreshColorProperty, nameof(BaseTheme.PullToRefreshColor))
                        .Bind(RefreshView.CommandProperty, nameof(ReferringSitesViewModel.RefreshCommand))
                        .Bind(RefreshView.IsRefreshingProperty, nameof(ReferringSitesViewModel.IsRefreshing)),
                    Constant(0), Constant(titleRowHeight), RelativeToParent(parent => parent.Width), RelativeToParent(parent => parent.Height - titleRowHeight))
                .Add(_isiOS ? new TitleShadowView(themeService) : null,
                    Constant(0), Constant(0), RelativeToParent(parent => parent.Width), Constant(titleRowHeight))
                .Add(_isiOS ? new TitleLabel() : null,
                    Constant(10), Constant(0))
                .Add(_isiOS ? closeButton : null,
                    RelativeToParent(parent => parent.Width - closeButton.GetWidth(parent) - 10), Constant(0), RelativeToParent(parent => closeButton.GetWidth(parent)))
                .Add(_storeRatingRequestView,
                    Constant(0), RelativeToParent(parent => parent.Height - _storeRatingRequestView.GetHeight(parent)), RelativeToParent(parent => parent.Width));
}

[brminnick]

Object Initialization: Classes vs Properties

Example Property

Label TitleLabel => new Label {
             Text = PageTitles.ReferringSitesPage
         }  .Font (family: FontFamilyConstants.RobotoMedium, size: 30)		
             .DynamicResource (Label.TextColorProperty, nameof(BaseTheme.TextColor))
             .Center () .Margins (top: titleTopMargin) .TextCenterVertical ();

Example Class

class TitleLabel : Label
{
    public TitleLabel()
    {
        Text = PageTitles.ReferringSitesPage;
        this.Font(family: FontFamilyConstants.RobotoMedium, size: 30);
        this.DynamicResource(TextColorProperty, nameof(BaseTheme.TextColor));
        this.Center().Margins(top: _titleTopMargin).TextCenterVertical();
    }
}

I prefer to use classes for the customized controls in lieu of an expression-bodied property because a Property in C# should be a data member of the class and it goes against C# norms to have a Property always instantiate a new instance of a class.

A work-around for this would be to use a Method that, similar to a Factory, denotes that a new object instance will be created each time it is called.

Example Method

Label CreateTitleLabel() => new Label 
{
    Text = PageTitles.ReferringSitesPage
}.Font (family: FontFamilyConstants.RobotoMedium, size: 30)		
 .DynamicResource (Label.TextColorProperty, nameof(BaseTheme.TextColor))
 .Center().Margins(top: titleTopMargin).TextCenterVertical();

Recommended Solution

The only reason I create these classes is because there isn't an extension method yet for every UI Property.

If Xamarin.Forms (and hopefully .NET MAUI) contains an extension method for each UI Property, we wouldn't need to worry about using Classes vs Properties vs Methods for initializing controls, because everything would be done in-line, like so:

Content = new Label()
   .Text(PageTitles.ReferringSitesPage)
   .Font (family: FontFamilyConstants.RobotoMedium, size: 30)		
   .DynamicResource (Label.TextColorProperty, nameof(BaseTheme.TextColor))
   .Center().Margins(top: titleTopMargin).TextCenterVertical();

[brminnick]

Build() vs readonly

I don't like using a Build() method because it breaks the ability to define UI controls as readonly fields.

Most UI controls in Xamarin.Forms are only instantiated once and the only way to prevent them from being re-instantiated is to use the readonly modifier.

When we use a method like Build() to instantiate the UI controls, we are no longer in the constructor and cannot use readonly.

[VincentH-Net]

Build() vs readonly

@brminnick yes, that is a disadvantage.
I added the Build() pattern so your app could have LiveSharp hot reload.

Which do you prefer, hot reload or readonly?

Also, if you use the constructor for creating the markup instead of a Build() method, you no longer have the option to move the page UI logic in the ctor to a separate partial class in a .logic.cs file. So less separation and less clean markup.

[VincentH-Net]

C# Markup: Pure Fluent vs Declarative Fluent & Unconventional Conventions

Thanks Vincent!

I added some more extension methods for RelativeLayout
...
Object Initialization: Classes vs Properties

You're welcome @brminnick ! It goes without saying (but hey I said it anyway) that I respect your preferences.

I'll explain below why I code C# Markup as in the above screenshot, which is consistent with my MAUI C# Markup proposal:

If I understand correctly, you prefer a pure fluent style, and don't mind repeating on each view new / Create, trailing () and often .Add():

Pure Fluent C#

new StackLayout()
    .Add(new Label()
        .Text("Hi")
        .Font(LargeTitle))
    .Add(new Button()
        .Text("Click Me"))
    .Add(CreateMyHeader())
    .Add(CreateMyBody())
    .Add(CreateMyFooter())

In declarative C# Markup I aim to remove noise. Less is more:

Declarative Fluent C#

StackLayout (
    Label ("Hi") 
        .Font (LargeTitle),
    Button ("Click Me"),
    MyHeader,
    MyBody,
    MyFooter
)

This makes C# markup read similar to succesful markup languages:

SwiftUI

VStack {
    Text ("Hi")
        .font(.largeTitle),
    Button() {
        Text ("Click Me")
    },
    MyHeader(),
    MyBody(),
    MyFooter()
}

XAML

<StackLayout>
    <Label
        Text = "Hi", 
        FontFamily = "LargeTitle" />
    <Button
        Text = "Click Me" />
    <control:MyHeader />
    <control:MyBody />
    <control:MyFooter />
</StackLayout>

I avoid repetition of keywords, name prefixes, parentheses and method calls like .Add(). In my experience these repetitions do not help devs to read, scan or understand the markup; they just get in the way. In general, where (markup) DSL's are compared, an important quality attribute is their signal / noise ratio.

To reduce noise, I use an optimized, but consistent set of coding and formatting conventions for declarative C# markup.
Note that readability is key to productivity; the avg ratio of time spent on reading / writing code = 90% / 10%.

Background:

In the years I worked with C# markup I found that C# conventions (both coding and formatting) have been developed primarily to benefit logic, but not declarative markup. This is perfectly logical ;-) when you use C# for logic and another language, like HTML or XAML, for declarative markup.

But the times they have changed and I found no reason to fully stick to all conventional logic conventions in declarative markup, where some of them work against the developer. Declarative markup differs fundamentally from logic in that it is both deeply nested and highly cohesive; that is why it benefits from it's own - slightly different - set of coding and formatting conventions.

I also thought of these C# language changes to make it support declarative markup better. Some of these C# changes I approximated by using existing C# in a slightly unconventional manner.

To conclude, my thinking is:

Coding and formatting conventions are here to serve developers, not the other way around. So where the conventions don't help we are free to change them; the goal is to make developing C# Markup a joy.

I propose to have the tooling support a specific, configurable set of conventions for declarative markup, to be applied automatically in declarative markup source only (auto format, code convention warnings and fixes/refactorings). C# Markup as a domain specific language deserves it's own conventions and tooling support. Of course the conventions for logic remain unchanged.

[VincentH-Net]

workaround ... ##Example Method

@brminnick using methods instead of properties to new up composite markup I can live with (although I like to reduce that () noise wherever I can). As soon as you need to pass parameters to it, it needs to become a method anyway.

I don't recommend prefixing every method with Create though, that adds a lot of repetition and noise.

Markup DSLs that are widely popular with developers, like Flutter or SwiftUI, don't do that. Neither does Comet / the MAUI MVU C# Markup. C# Markup has similar conventions to those.

It may help to consider that in terms of C# context the entire markup is created within a single object initializer.
C# syntax within an object initializer is already a special case; things are already possible there that cannot be done elsewhere, e.g. omitting repeated instance. for every property assignment.

So it is not that big of a step to omit repeating the new keyword in an object initializer, because the whole expression is within the context of a single new.

Hence it is also acceptable to not prefix methods with Create when they are intended for use in an object initializer.

In general, DSL methods are best designed for the context of their intended use - e.g. in Label().Color().Black() the .Black() method is part of a fluent subchain, which means that .Black() can only be used after .Color(), so it is not good design to name it .SetColorToBlack() - that would be ignoring a known usage context and by that introduce unneeded repetition and noise.

[VincentH-Net]

The only reason I create these classes is because there isn't an extension method yet for every UI Property.

If Xamarin.Forms (and hopefully .NET MAUI) contains an extension method for each UI Property, we wouldn't need to worry about using Classes vs Properties vs Methods for initializing controls, because everything would be done in-line

@brminnick It is in the MAUI C# Markup proposal that all properties (except the ones set in factory method parameters) have extension methods.

But that will not change the need to structure and name composite markup. Without that you would need to create markup for large pages in one humongous expression of low-level views, without any names in it. That would necessitate inline comments like // Header, // Footer instead of just using Header(), Footer(), which lets you enjoy the navigation and collapse benefits that the editor gives you for methods.

So the need for named methods to structure your markup is unchanged by that.