Code Usability

[robertmclaws]

This is a really great library, and kudos to you @andrewbares for writing it. I've been using it for several months now, and while it does the job well, sometimes it's not terribly friendly to use to build complicated layouts. I have some ideas on how to make that better, if you don't mind.

• Some basic constructors would go a long way towards reducing the amount of space this code takes up, and would take some of the guesswork out of what is required and what isn't.
• Some factory methods would be useful if all you want to do is spit out a Peek image and a couple of lines of text.
• Some helper methods for managing the TileUpdateManager lifecycle and accepting these objects w/o dumping them to XML first would help devs not have to repeat code for every app they write.

I'm submitting a PR for a couple of these issues based on some of the objects I've been working with, as a jump-start. I hope they help.

[andrewleader]

Hey! Thanks for the additions and pull request!

I definitely like the idea of some helpful constructors and factory methods... My fear with helper methods is just that they might not be needed by everyone (and adds additional code to maintain).

But I totally agree that constructing a simple toast notification, for example, is needlessly complicated as it stands today, and I definitely like the idea of improving that.

So... 100% with you on the spirit of improving things :) I'm just slightly fearful of introducing too many helpers/constructors, but if the community agrees that additional constructors are beneficial and not a burden, I can easily be persuaded! The constructors are definitely useful if you're using JavaScript or C++, since those languages don't have the handy object initializer syntax like C# does.

Also - some news - We're moving this project to a new location shortly (we've already moved the existing code), so I'm not going to do anything with your pull request now... and once that new project is live, you can make a pull request there, and then you'll get credit for your contribution too :)

[robertmclaws]

Cool. thanks for letting me know.

FYI, with this PR, I tried to keep the constructors tightly-focused to common use cases. They mostly stemmed from the "factory methods" in my live tile helper class that I use to kick out these objects.

For example, I didn't check this ability into your codebase, but I have a function that can basically do this:

    return GetPeekBinding(
        $"{(int)pokemon.PokemonId}.png",
        GetCenteredAdaptiveText(Resources.Pokemon.GetString(pokemon.PokemonId.ToString()), AdaptiveTextStyle.Body),
        GetCenteredAdaptiveText($"CP: {pokemon.Cp}"),
        GetCenteredAdaptiveText($"HP: {(pokemon.Stamina / pokemon.StaminaMax) * 100}%")
    );

The first parameter is the peek image location, and the next parameter is actually a params that can pass in as many objects that get dropped into the "Children" collection that I want. If I changed it to use the constructors in that PR, I could eliminate some code in my helpers, and it would look like this:

    return GetPeekBinding(
        $"{(int)pokemon.PokemonId}.png",
        new AdaptiveText(Resources.Pokemon.GetString(pokemon.PokemonId.ToString()), AdaptiveTextStyle.Body, AdaptiveTextAlign.Center),
        new AdaptiveText($"CP: {pokemon.Cp}", AdaptiveTextAlign.Center),
        new AdaptiveText($"HP: {(pokemon.Stamina / pokemon.StaminaMax) * 100}%", AdaptiveTextAlign.Center)
    );

It's not a huge change for me, but the less code I have to maintain in my own projects, the better :). Anyways, thanks for all your hard work. This is an incredibly useful library :).

[andrewleader]

Wait, why not use the object initializer syntax?

    return GetPeekBinding(
        $"{(int)pokemon.PokemonId}.png",
        new AdaptiveText()
        {
            Text = Resources.Pokemon.GetString(pokemon.PokemonId.ToString()),
            HintStyle = AdaptiveTextStyle.Body,
            HintAlign = AdaptiveTextAlign.Center
        },
        new AdaptiveText()
        {
            Text = $"CP: {pokemon.Cp}",
            HintAlign = AdaptiveTextAlign.Center
        },
        new AdaptiveText()
        {
            Text = $"HP: {(pokemon.Stamina / pokemon.StaminaMax) * 100}%",
            HintAlign = AdaptiveTextAlign.Center
        }
    );

And I believe WinRT would unfortunately have an issue with some of those constructors, since JavaScript can't differentiate by type (you can't have multiple constructors with the same number of parameters, since JavaScript can't tell if it's getting an AdaptiveTextAlign or AdaptiveTextStyle). So we would either have to keep those specific constructors out of the WinRT version of the library, or remove any constructors that have the same number of parameters.

FYI, the new library that Extensions has moved to is the UWP Community Toolkit :) https://github.com/Microsoft/UWPCommunityToolkit

[robertmclaws]

Because that's like 9 unnecessary lines of code. It doesn't need to be that drawn out.

I can probably streamline the constructors a bit. There still might be some that don't work in JS thought... those could probably just be left out.

Thanks for letting me know, I'll be sure to do the PR over there. Thanks!

[andrewleader]

I mean, I like the idea... but the flaw is when you start to consider all the various combinations of all the properties of AdaptiveText... what if someone needs HintWrap but not HintAlign and HintStyle? Then you have a constructor that takes (Text, HintWrap) in addition to (Text, HintAlign) and (Text, HintStyle). And what if they also need HintStyle with HintWrap? The combinations are quite numerous.

I feel like object initializer syntax was invented for this exact reason...

We theoretically could have a constructor that just takes all optional parameters, but at that point we're just re-creating object initializer syntax...

Also, yes that's 9 additional lines of code, but I would argue it's much more readable, which is great if you're working on a project with others.

[robertmclaws]

And see, I would argue it's actually less readable. All of the brackets and indents make it harder to follow in the MSDN documentation... you have to scroll in both directions.

It's a point of preference. And that's why I think it should work both ways.

The point of constructors would be not hit every scenario, but the most common ones. AdaptiveText requires text to be displayed. That should be in a constructor. PeekImage requires at least one image. That should also be in a constructor.

Constructors help take the guesswork out of what's important in an object, and what isn't. I would say that is the main thing that these extensions lack, is context. I HAVE to read the docs, because I can't just get a pre-constructed PeekTile and just supply it with an image and a string.