Thanks @Eel2000!

Inheritance / Implementations

public class RatingView

What does RatingView inherit / implement? E.g. Will it inherit from View? Will it implement IAnimatable?

We should inherit from the highest possible .NET MAUI control (typically View or Element), and we should support every .NET MAUI interface that aligns with the functionality of the custom control. This ensures that we cover every desired scenario that our users may need.

As an example, check out Popup's inheritance:

RatingShape Extensibility

I see that RatingShape is an enum. This isn't a bad thing, however it limits our users; they won't be able to create their own custom shapes.

And as maintainers, this means that we'll be required to add new shapes to unblock users.

Perhaps we could use string instead, and provide the following const values in public static class RatingViewShapes. This way, users can implement their own custom shapes without needing to wait for us to publish new shapes to a new release.

public class RatingView
{
  public string Shape { get; set; } = RatingViewShapes.Star;
}

public static class RatingViewShapes
{
  public const string Star = nameof(Star);
  public const string Heart = nameof(Heart);
  public const string Like = nameof(Like);
  public const string Dislike = nameof(Dislike);
}

Naming Suggestions

public enum RatingShapes

If we decide to move forward with an enum, let's rename it to RatingShape

public bool Animate

Let's rename this to ShouldAnimate. The name of a bool should be a question that its value answers; e.g. ShouldAnimate = true

public bool AllowRating

What does this boolean do? I'm a bit confused by its name because if RatingView.AllowRating == false, then doesn't that mean RatingView won't show a rating, defeating the purpose of RatingView? This should also be named as a question, eg ShouldAllowRating.

public Color Fill { get; set; }

public Color EmptyColor { get; set; }

I assume that these do the same, but opposite, tasks: Fill is the background color when the rating is selected / filled, and EmptyColor is the background color with the rating is not selected / not filled.

If so, let's rename them to align better with each other. This will help signal to users that they do the same thing. Perhaps FilledBackgroundColor and EmptyBackgroundColor?

//the number of shapes filled to show the rating(default)
public double Value { get ; set ; }

Let's use a more descriptive name. As a rule of thumb, if you have to leave a comment describing what a property does, it likely has a poor name.

Maybe public double CurrentRating { get; set; }?

//number of shapes (clickable or not) to show on the view(page) 
public int Maximum { get; set; }

Let's use a more descriptive name. As a rule of thumb, if you have to leave a comment describing what a property does, it likely has a poor name.

Maybe public double MaximumRating { get; set; }?

Minimum Rating

Since we have Properties for MaximumRating and CurrentRating, let's add the property MinimumRating.

This would allow users to set a minimum value to the rating. I could see users wanting like 1 Star to be a minimum value in a 5-star rating as opposed to 0 Stars.

Custom Animation

Since we have bool ShouldAnimate, let's allow the user to provide a custom animation:

public static readonly BindableProperty CustomAnimationProperty;

public Animation? CustomAnimation;

When CustomAnimation is null, we'll default to the platform-default.

ICommand Nullability

The default value for both Command and CommandParameter is null, so let's make both properties nullable:

public ICommand? Command { get; set; }
public object? CommandParameter { get; set; }

BindControl

What is BindControl and what does it do? Should it also be nullable?

public object BindControl { get; set; }

Couple of solutions on points raised by @brminnick

I see that RatingShape is an enum. This isn't a bad thing, however it limits our users; they won't be able to create their own custom shapes.

I really like the idea of having it as enum, since it's hard for devs to do a typo and xaml IntelliSense will work. We can create a SourceGenerator to generate this enum, and have an attribute that will allow users to add more fields to it.

When CustomAnimation is null, we'll default to the platform-default.

There's no platform-default, if I did look right the implementation is based on the shared layer using drawing controls. I know that Android has this View implemented, but not other platforms... So we need to think if we go all shared or use Android as native and build other platforms to match its behavior.

assume that these do the same, but opposite, tasks: Fill is the background color when the rating is selected / filled, and EmptyColor is the background color with the rating is not selected / not filled.

Should those be nullable as well?

Element

Thank you @brminnick

Inheritance

For the inheritance, I'm still trying to see which high MAUI control it should inherit from as the original code was inheriting the Grid Element.

RatingShape Extensibility

Concerning the RatingShape, i like the @pictos Idea to write a source generator. the only issue is, I don't have any experience in it. I may need help with it.

I thing that it is a great opportunity for me to improve the control and make it more interesting.

Custom Animation

It should be nullable. If the user chooses not to provide any value, the control should not animate at all. However, at this point, I guess, we should consider implementing default options for other platforms as previously specified by @pictos.

Tanks @Eel2000! Could you please edit the Detailed Design above to include the recommendations we've discussed thus far?

Inheriting from Grid

Ah yea, inheriting from any Layout, like Grid, brings about a few challenges that you'll need to account for in RatingView.

For example, every Grid has the property Children. But in our implementation of RatingView, we wouldn't allow the user to add/remove any Children manually (our implementation of RatingView will populate the Children for them automatically based on the MinimumRating/MaximumRating values and the RatingShape requested), so we'd want to block them from modifying the Children property.

We can accomplish this by leveraging the new keyword to override the public property public IList<IView> Children with our own implementation of public new IReadOnlyList<IView> Children; this way, we still provide read-access to the Children list, but don't allow external users to add/remove any items from the list.

👇 Something like this

public class RatingView : Grid
{
    // ...

    public new IReadOnlyList<IView> Children
    { 
        get => base.Chilrdren;
        private set => base.Children = value;
    }
}

Source Generators

I don't recommend you do anything with Source Generators for the first implementation of RatingView. They're very difficult to learn. I've seen a lot of well-intentioned devs go that path and fail.

To keep things moving forward, let's do this:

1. Let's move forward with using an enum for the RatingShape
   • We'll include in the docs that only Star, Heart, Dislike and Like are available shapes in this first release of RatingView
2. Once this RatingView is merged, you should open a new Proposal for adding extensibility to public enum RatingShape via Source Generators.

Actually RatingView doesn't inherit from Grid, but from TemplatedView which makes sense for this control

Here's the BaseTemplateView implementation.

They do expose the Children property because TemplatedView is a layout, but it's marked to not show at IntelliSense (but somehow, Rider is showing it). Also, since it's an IReadOnlyList we can't set or add a value to it, so we're safe.

I like the idea of this Control. I would like to add to the discussion around the Shape. Instead of an Enum or just a string, I would also like to propose to make it of type Shape and let the user select a Shape of his own. We could add some Shapes as part of this feature, so that "usual" shapes are already available for the user and they can pick from them, but it would give us the extensibility to use any shape.

Approve. But please update the issue with the final design.
From the discussion, we shouldn't use BaseTemplateView (Because it uses Compatibility library) and Grid (Because we shouldn't allow to add any child), Shape (Enum or type Shape to allow any figure)

Approve. But please update the issue with the final design. From the discussion, we shouldn't use BaseTemplateView (Because it uses Compatibility library) and Grid (Because we shouldn't allow to add any child), Shape (Enum or type Shape to allow any figure)

Alright. It will be done. thanks

From my experience in implementing the AvatarView, I tried implemented a number of different inherited base classes, including TemplatedView, Grid, Frame, just to mention a few. I ended up deciding to use Border as I looked at how other composite controls were implemented in .NET MAUI specifically (such as Button, RadioButton, etc.) and Border was the closest to the needs of the proposed control. I did inherit other .NET MAUI interfaces to provide greater control to the end user (such as IBorderElement).

BTW: I did implement in the Samples app for the AvatarView some uses as a Ratings control. However, I do think a dedicated and enhanced control is more appropriate.

I think we should *allow users to provide their own custom shapes, should they choose to do so (and if they have not already selected a pre-defined enum shape). The Syncfusion Rating implementation allows this (by default they provide Star, Heart, Circle, Diamond). Something like this taken from the .NET MAUI Border implementation, or look at the AvatarView samples for ideas.

		/// <summary>Bindable property for <see cref="StrokeShape"/>.</summary>
		public static readonly BindableProperty StrokeShapeProperty =
			BindableProperty.Create(nameof(StrokeShape), typeof(IShape), typeof(Border), new Rectangle(),
				propertyChanging: (bindable, oldvalue, newvalue) =>
				{
					if (newvalue is not null)
						(bindable as Border)?.StopNotifyingStrokeShapeChanges();
				},
				propertyChanged: (bindable, oldvalue, newvalue) =>
				{
					if (newvalue is not null)
						(bindable as Border)?.NotifyStrokeShapeChanges();
				});

I also think we need a property to indicate the precision of fill, such as Full (1,2,3,4 or 5), Half (0.5, 1, 1.5, 2, etc) or precise (0, 0.1, 0.2, ... 4.8, 4.9, 5.0).

I know this all takes time and effort, but I do think it's personally worthwhile and of great benefit to the community.

Thank you @GeorgeLeithead , I'm still working on the control and I do much appreciate your suggestion about the full or half fill options I forgot about that.

for the Inheritance i think for now as i haven't fount another to do it without using templateview , I will still using the border element.