-
Notifications
You must be signed in to change notification settings - Fork 389
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] New rating component #2258
Conversation
This is great! Has been on my list of things to add for quite some time. Will do a full review soon. Couple of quick remarks.
Looking forward to getting this in! |
For CoreIcons I copied the previous line. There is an error in the previous line. I fixed it. |
Great job :-) Could you check all the properties inherited from I will verify the code more in detail later :-) |
Adding this component in the page |
when the pointer is on a star the previous ones and this one become full, the following ones empty |
Why this comment? I don't understand :-) This effect does not occur with MudBlazor. |
One other thing...we have a pretty high bar on accessibility so adding keyboard support like the MudBlazor one has should be included as well. We can use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you think Size24 is too big for the icons? If not, we could change the component to also support half values for a rating. (StarHalf icon is not available in Size20). Would of course require more change like FluentInputBase<double>
, etc but think it could be valuable.
We have an internal React rating component version that also supports half ratings
IMO for now we could ignore the half values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check examples in FluentButtonTests.razor
[Fact]
public void FluentButton_Title()
{
// Arrange && Act
var cut = Render(@<FluentButton Title="My Title">My button</FluentButton>);
// Assert
cut.Verify();
}
@franklupo Both Denis and I are creating a lot of comments/remarks/suggestions. Please, please do not see this as criticism. We are really happy with your contributions. We are just trying to look after quality and coding standards (and can be real nitpickers about that sometimes 😑). |
I understand, no problem. I have a lot to learn. |
I'm having trouble with tests. Could you help me create one? |
Sure. I will create some Unit Tests later in the day 😊 |
Example of
Your job:
@using Xunit;
@inherits TestContext
@code
{
public FluentRatingTests()
{
JSInterop.Mode = JSRuntimeMode.Loose;
Services.AddSingleton(new LibraryConfiguration());
}
[Fact]
public void FluentRating_Empty()
{
// Arrange && Act
var cut = Render(@<FluentRating />);
// Assert
cut.Verify();
}
[Fact]
public void FluentRating_Value()
{
// Arrange && Act
var cut = Render(@<FluentRating MaxValue="10" Value="2" />);
// Assert
cut.Verify();
}
[Fact]
public void FluentRating_SelectValue()
{
int value = 0;
// Arrange
var cut = Render(@<FluentRating @bind-Value="@value" />);
// Act: Click on the second star
cut.FindAll("svg").ElementAt(1).Click();
// Assert
Assert.Equal(2, value);
}
[Fact]
public void FluentRating_AllowResetFalse()
{
int value = 0;
// Arrange
var cut = Render(@<FluentRating @bind-Value="@value" AllowReset="false" />);
// Act: Click twice on the second star
cut.FindAll("svg").ElementAt(1).Click();
cut.FindAll("svg").ElementAt(1).Click();
// Assert
Assert.Equal(2, value);
}
[Fact]
public void FluentRating_AllowResetTrue()
{
int value = 0;
// Arrange
var cut = Render(@<FluentRating @bind-Value="@value" AllowReset="true" />);
// Act: Click twice on the second star
cut.FindAll("svg").ElementAt(1).Click();
cut.FindAll("svg").ElementAt(1).Click();
// Assert
Assert.Equal(0, value);
}
} |
Could you tell us when your PR will be finished (all discussions resolved)? |
If I haven't forgotten anything, that's fine with me. |
And Unit Tests (to have 80%) and the Accessiblity issue ? |
@franklupo thanks again for your contribution! We are merging this in now and will make some final tweaks before release. |
To add aria-label="@CurrentValue" and the new unit test do I need to create a new PR? |
No, all good. Doing some final tweaks and this is one of them. |
@franklupo I don't know why (not yet check), but this component is instable (flicker effect) :-( You can go to https://preview.fluentui-blazor.net/Rating to reproduce this behavior. FluentRating-Instable.mp4 |
I'm working on it and I think I've found a solution. |
Rating component
📖 Description
Rating component like https://mudblazor.com/components/rating