Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rating view #1706

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Rating view #1706

wants to merge 11 commits into from

Conversation

Eel2000
Copy link

@Eel2000 Eel2000 commented Feb 22, 2024

Description of Change

Adding the Rating view to the community toolkit, this is the initial code which requires some validations.
this code was ported from an existing repo with changes made based on suggestions from the team. It needs some validations(suggestions) before going any further. Unit tests, samples, and the code doc consolidation are not yet. Will be added after reviews and any new suggestion.

Linked Issues

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)
    Unit tests omitted because i wanted to validate first the existing code which was ported from an existing repo before writing
    unit tests.
  • Has samples (if omitted, state reason in description)
    Need some code validation before adding the sample page in the samples app.
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

@Eel2000 Eel2000 requested review from brminnick and pictos and removed request for pictos February 22, 2024 14:33
Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving this forward, some code style points

Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Eel2000! Could you fix these Find + Replace typos?

Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Eel2000!

We should be using read-only properties instead of expression-bodied properties.

I also noted that we still need to add descriptive XML comments, but we should wait to complete these until the Proposal has been approved.



/// <summary>Star Color Bindable property.</summary>
public static readonly BindableProperty FilledBackgroundColorProperty = BindableProperty.Create(nameof(FilledBackgroundColor), typeof(Color), typeof(RatingView), defaultValue: Colors.Yellow, propertyChanged: OnBindablePropertyChanged);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is preferable Backgroud as Brush or BackgroundColor as Color?
Background allows more customization like gradient background

@Eel2000 Eel2000 self-assigned this Apr 22, 2024
@Eel2000
Copy link
Author

Eel2000 commented Apr 22, 2024

@dotnet-policy-service agree

@Eel2000 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@Eel2000 Eel2000 requested a review from brminnick April 22, 2024 13:11
@Eel2000 Eel2000 requested a review from pictos April 29, 2024 12:50
@dotnet-policy-service dotnet-policy-service bot added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] RatingView proposal
4 participants