Rating view

[GeorgeLeithead]

Description of Change

This pull request introduces a custom RatingView control, designed to provide developers with a flexible and customizable rating mechanism, similar to those used on popular review and feedback platforms. The control is versatile and adaptable, allowing developers to tailor it to the specific needs of their applications through a variety of customizable properties.

The primary goal of this control is to enhance the user experience by enabling precise and intuitive user ratings, while offering developers full control over the visual and functional aspects of the control. The following are key features and properties available in the RatingView control:

Key Features and Property Descriptions:

• Set the Maximum Number of rating items:
  Developers can define the total number of items (e.g., stars, hearts, etc., or custom shapes) available for rating. This allows for ratings of any scale, such as a 5-star or 10-star system, depending on the needs of the application.

• Set the current Rating:
  The control supports setting the current rating value, allowing for both pre-defined ratings (e.g., from previous user input or stored data) and updates during runtime as the user interacts with the control.

• Set the Filled (rated) background colour:
  Developers can set the colour that will be applied to the filled (rated) portion of each item, offering flexibility in defining the visual aesthetic of the rating items when selected by the user.

• Set the Empty rating colour:
  The colour for the unfilled (empty) rating items can also be customized. This allows for clear visual differentiation between rated and unrated items.

• Set the rating item shape Border Colour and Thickness:
  The control allows for defining the border colour and thickness of each rating item. This provides additional flexibility to create visually distinct and stylized rating shapes with custom borders.

• Set the rating Fill type (Shape or Item):
  Developers can choose how the fill is applied against the entire rating item or just the shape, enabling more nuanced visual presentation, such as filling only the interior of stars or the full item.

• Set the rating item Shape:
  The control provides the ability to define the shape of the rating items, such as stars, circles, like, dislike, or any other commonly used rating icons.

• Define a Custom rating item shape:
  In addition to standard shapes, the control allows for defining custom shapes for rating item shapes. This feature empowers developers to implement unique designs, such as distinctive symbols, as rating items.

• Set the rating item shape Size:
  The size of the rating item shape can be customized to fit the overall design of the application, providing the flexibility to adjust the control to various UI layouts.

• Set the Spacing between rating items:
  Developers can define the spacing between individual rating items, ensuring proper alignment and spacing based on the control’s context within the application's layout.

• Set the Padding between the rating item and the rating item shape:
  Padding between the rating item and its corresponding shape can be adjusted, allowing for finer control over the appearance and layout of the rating items.

• Attach a Handler to the RatingChanged Event:
  The control supports event handling through the RatingChanged event, enabling developers to respond to changes in the user’s rating input, such as updating data models, triggering animations, or submitting ratings to a backend service.

• Define the control in XAML or C# syntax:
  The RatingView control can be defined and implemented using either XAML or C# syntax, making it highly versatile for both declarative and programmatic UI design, depending on developer preference or application architecture.

Linked Issues

• Fixes [Proposal] RatingView proposal #1607

PR Checklist

• Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
• Has tests (if omitted, state reason in description)
• Has samples (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Changes adhere to coding standard
• Documentation created or updated: .NET MAUI Community Toolkit - RatingView MicrosoftDocs/CommunityToolkit#472

Additional information

This pull request also includes three sample pages to assist developers in exploring and implementing the RatingView control:

• XAML Syntax Sample Page:
  Demonstrates the usage and configuration of the RatingView control using XAML, providing an example for developers who prefer a declarative approach to UI development.

• C# Syntax Sample Page:
  Shows how the RatingView control can be implemented programmatically in C#, catering to developers who prefer to work in code for dynamic or logic-driven UIs.

• Showcase Page:
  Provides a demonstration of various customization options and use cases for the RatingView control, offering developers inspiration on how they can integrate and style the control in their applications. This page highlights the control's flexibility in different scenarios and showcases potential real-world applications.

Platforms tested

• Android
• Windows
• MacCatalyst
• iOS
• Tizen

NOTE: Due to the developer not having access to iOS, MacCatalyst or Tizen, these platforms have not been tested or validated as working or visually correct.

[brminnick]

Why is our maximum bound set to 25? Is this a platform-specific problem?

Or are we just picking an arbitrary number for a maximum?

I'm totally fine with us setting a maximum value, but I'm just curious if it's a platform constraint or an artificial constraint set by us?

[brminnick]

Thanks @GeorgeLeithead! I LOVE that you included a C# Markup Example. I've re-factored it to follow best practices:

• Flattened layouts (eg avoid putting a HorizontalLayout inside of a Grid)
• Use extension methods when available (e.g. .Text(), CenterVertical etc)
• Updated the Bindings
  • Use TypedBindings: https://learn.microsoft.com/en-us/dotnet/communitytoolkit/maui/markup/extensions/bindable-object-extensions#bind
  • Use [NotifyPropertyChangedFor] in ViewModel
• Remove Unneeded EventToCommandBehavior
  • The previous solution worked, but added layers of complexity
  • Obsoleted by [NotifyPropertyChangedFor]
• Remove Auto from RowDefinitions + ColumnDefinition
  • Auto causes massive performance impacts because MAUI must recalculate every row in the entire Grid when one row's size increases when its RowDefinition is Auto

I haven't done a deep dive into the RatingView source code yet, but I did make some tweaks while updating the C# Markup Sample page:

• Rename Shape -> ItemShape
  • This helps ensure users know that they're setting the shape of the items in the RatingView to ensure they don't assume that Shape means the shape of the RatingView itself
• SizeProperty -> ItemShapeSizeProperty
  • This helps ensure users know that they're setting the size of the items in the RatingView to ensure they don't assume that Size means the size of the RatingView itself
• Change RatingView.Control from public -> internal
  • I don't understand TemplatedView super well, but I don't think the user will ever need to (or should) manually change this value
• Use int for MaximumRating and Rating
  • This helps ensure that users using intelligence understand that we expect ad integer number for these values
  • byte allowed them to pass in unsupported values
• Add bounds to ShapeBorderThickness
  • This ensures we don't get negative values for border thickness
  • I added Unit Tests to support this scenario
• Add bounds to MaximumRating
  • This ensures we don't get invalid values for MaximumRating
  • I added Unit Tests to support this scenario
• Add bounds to Rating
  • This ensures that Rating never exceeds MaximumRating
  • I added Unit Tests to support this scenario
• Remove static modifier from RatingView.weakEventManager
  • When it was static, it would cause event MaximumRating to fire on all RatingView instances when MaximumRating changed on one instance
• Removed Null Coalescing Operator (!)
  • We need to always check for null; we should never assume an instance is not null
  • The ! is a bit of an anti-pattern and I don't recommend any C# developer use it
• Removed Debug.Assert
  • Instead of using Debug.Assert to ensure an instance is not null, we should write our own logic that helps explain why we're checking for null and our expected results
  • Debug.Assert doesn't run in RELEASE configuration (only in DEBUG configuration)
• Add [NotNull] to EmptyColor, FilledColor, and ShapeBorderColor
  • This allows users to pass in null for the value of these properties while still maintaining our promise that the value will never be null when they retrieve its value
  • We then check for null in the setter and change it Colors.Transparant before passing it along to the BindableProperty in SetValue()
  • This will improve the XAML user experience where Color can default to null

[GeorgeLeithead]

• Rename Shape -> ItemShape

  • This helps ensure users know that they're setting the shape of the items in the RatingView to ensure they don't assume that Shape means the shape of the RatingView itself

• SizeProperty -> ItemShapeSizeProperty

  • This helps ensure users know that they're setting the size of the items in the RatingView to ensure they don't assume that Size means the size of the RatingView itself

• Change RatingView.Control from public -> internal

Naming is hard... Like most development, it started off with a very different implementation and has morphed into what it is now. Happy with the name changes. I'll need to update the Docs, which have already been written.

[GeorgeLeithead]

• Use int for MaximumRating and Rating

  • This helps ensure that users using intelligence understand that we expect ad integer number for these values
  • byte allowed them to pass in unsupported values

Not sure I agree here. The System.Byte integral type is the smallest POSITIVE unsigned 8-bit integer available. Whereas the System.Int integral type supports NEGATIVE and is a signed 32-bit integer, therefore the possible values are huge.

Yes, I know we have added in ValidateValue handlers, but as an end user who will just see the data type of int may just assume it is the full signed 32-bit integer and offer negative and positive values. Whereas, if they see it is a byte, this already draws the developer in to know that the value is a positive and restricted.

[GeorgeLeithead]

• Remove static modifier from RatingView.weakEventManager

  • When it was static, it would cause event MaximumRating to fire on all RatingView instances when MaximumRating changed on one instance

Thanks for this. I wasn't sure why I was experiencing this in some early 'samples'. I think it was mostly negated in the samples, when I added IsReadOnly.

[GeorgeLeithead]

• Add bounds to MaximumRating

  • This ensures we don't get invalid values for MaximumRating
  • I added Unit Tests to support this scenario

• Add bounds to Rating

  • This ensures that Rating never exceeds MaximumRating
  • I added Unit Tests to support this scenario

Not sure I agree with this, and I feel that my original implementation was better. In that values outside the range (1 to 25) would be set to the nearest valid value, and not throw errors. Which is the SAME as Opacity and other common base properties.

[GeorgeLeithead]

Add bounds to ShapeBorderThickness

• This ensures we don't get negative values for border thickness
• I added Unit Tests to support this scenario

Not sure I agree here. Delving into .NET MAUI and looking at BorderElement and specifically the BorderWidthProperty, it is defined as a double and never validated as a positive (that I can find in the source code). As such, I think that we should be looking to do the same as the core .NET MAUI controls.

[GeorgeLeithead]

Removed Null Coalescing Operator (!)

• We need to always check for null; we should never assume an instance is not null
• The ! is a bit of an anti-pattern and I don't recommend any C# developer use it
  Removed Debug.Assert
• Instead of using Debug.Assert to ensure an instance is not null, we should write our own logic that helps explain why we're checking for null and our expected results
• Debug.Assert doesn't run in RELEASE configuration (only in DEBUG configuration)

How do we unit test these, as the Code Coverage report has now dropped from 100% to 85.9%? All branches should be covered IMO, and if not covered are they needed?

Is it even POSSIBLE to get to code such as this, and if not, is it even needed?

		var ratingView = (RatingView)bindable;

		if (ratingView.Control is null)
		{
			return;
		}

[GeorgeLeithead]

Remove Auto from RowDefinitions + ColumnDefinition

• Auto causes massive performance impacts because MAUI must recalculate every row in the entire Grid when one row's size increases when its RowDefinition is Auto

How does this handle when the user has their device set to anything other than the default "Font Size" and "Display size"? How does this work for our visual impaired users? Just increasing the "Display Size" by 1 setting, and most of the controls become un-usable!

[brminnick]

How do we unit test these, as the Code Coverage report has now dropped from 100% to 85.9%? All branches should be covered IMO, and if not covered are they needed?

As a general rule, if the Unit Test isn't covering a branch you've either written the library incorrectly or you need to add more unit tests. In this specific instance, our Unit Tests are still valid as long as we initialize RatingView.Control.

In the Code Coverage report for this PR, I see that our Unit Tests cover 94.27% of the lines in RatingView and
100% of the lines in RatingViewElement (screen shot below).

Can you help me understand where you're seeing that we only cover 85.9% of RatingView?

[bijington]

What's the thinking behind having a command here? Can't the value just be data bound to handle it changing?

[bijington]

Removed Null Coalescing Operator (!)

• We need to always check for null; we should never assume an instance is not null

• The ! is a bit of an anti-pattern and I don't recommend any C# developer use it

Removed Debug.Assert

• Instead of using Debug.Assert to ensure an instance is not null, we should write our own logic that helps explain why we're checking for null and our expected results

• Debug.Assert doesn't run in RELEASE configuration (only in DEBUG configuration)

How do we unit test these, as the Code Coverage report has now dropped from 100% to 85.9%? All branches should be covered IMO, and if not covered are they needed?

Is it even POSSIBLE to get to code such as this, and if not, is it even needed?

		var ratingView = (RatingView)bindable;



		if (ratingView.Control is null)

		{

			return;

		}

Could you achieve the passing of values to inner controls through bindings rather than doing it this way? Then you don't need to worry about checking for null

[GeorgeLeithead]

As a general rule, if the Unit Test isn't covering a branch you've either written the library incorrectly or you need to add more unit tests. In this specific instance, our Unit Tests are still valid as long as we initialize RatingView.Control.

In the Code Coverage report for this PR, I see that our Unit Tests cover 94.27% of the lines in RatingView and 100% of the lines in RatingViewElement (screen shot below).

Can you help me understand where you're seeing that we only cover 85.9% of RatingView?

I was looking at the Branch Coverage.

for example:

I'll 'try' and see if I can get these covered.