Open Eel2000 opened 11 months ago
Thanks @Eel2000!
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:
https://github.com/CommunityToolkit/Maui/blob/02896d32571af05ede9df8d6b10b32db174e15f6/src/CommunityToolkit.Maui/Views/Popup/Popup.shared.cs#L13
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);
}
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; }
?
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.
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.
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; }
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
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.
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.
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?
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.
:point_down: Something like this
public class RatingView : Grid
{
// ...
public new IReadOnlyList<IView> Children
{
get => base.Chilrdren;
private set => base.Children = value;
}
}
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:
Star
, Heart
, Dislike
and Like
are available shapes in this first release of RatingView
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.
Feature name
RatingView
Link to discussion
https://github.com/CommunityToolkit/Maui/discussions/1495
Progress tracker
Summary
Maui.RatingView - is a cross platform plugin for MAUI which allow you to use the rating capabilities in your application with ease and flexibility
Motivation
This feature aimes to add the rating capability in a maui application
Detailed Design
Usage Syntax
Here is a usage examples
Drawbacks
No response
Alternatives
No response
Unresolved Questions
No response