Skip to content

Conversation

@LuohuaRain
Copy link
Contributor

@LuohuaRain LuohuaRain commented Jul 23, 2024

Pull Request

📖 Description

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

@LuohuaRain LuohuaRain changed the title FluentSlider: by using web components current-value instead of value [FluentSlider] by using web components current-value instead of value Jul 23, 2024
@vnbaaij
Copy link
Collaborator

vnbaaij commented Jul 23, 2024

Sorry, I don't understand the title of the PR. Can you explain what this is fixing?

@LuohuaRain
Copy link
Contributor Author

Sorry, I don't understand the title of the PR. Can you explain what this is fixing?

Sorry, I have made modifications to this component by referring to the title of PR #1576

Currently, binding the value property does not seem to support two-way data binding effectively. Specifically, when the value is updated externally, the UI does not reflect these changes. By switching to current-value, the correct behavior is achieved.

@LuohuaRain
Copy link
Contributor Author

LuohuaRain commented Jul 23, 2024

https://explore.fast.design/components/fast-slider

image

there is no value, only current-value.

So I think it should be current-value

Could you please suggest a more suitable title? I'm not good at English😭

@dvoituron
Copy link
Collaborator

dvoituron commented Jul 23, 2024

  1. Did you have an issue with the current development?
    Because, I added this code in the Demo sample and all work fine.

    <FluentButton OnClick="@(e => { value = 50;} )">Click</FluentButton>
  2. We are using Fluent WebComponents v2 and the Slider contains a value attribute.
    See https://github.com/microsoft/fluentui/blob/web-components-v2/packages/web-components/src/slider/slider.vscode.definition.json

  3. Could you add this Unit Test to validate your change (to verify the binding)

[Fact]
public void FluentSlider_Binding()
{
    // Arrange
    int value = 3;
    var cut = Render(@<FluentSlider Min="0" Max="10" Step="1" @bind-Value="@value"></FluentSlider>);

    // Act
    var slider = cut.Find("fluent-slider");
    slider.TriggerEvent("onsliderchange", new ChangeEventArgs() { Value = 2 });

    // Assert
    Assert.Equal(2, value);
}

@vnbaaij
Copy link
Collaborator

vnbaaij commented Jul 23, 2024

Going with the general web components way of working, they use value only for setting the initial value. current-value should represent the current value (ie after binding)

@vnbaaij vnbaaij changed the title [FluentSlider] by using web components current-value instead of value [FluentSlider] Use current-value instead of value attribute in web component Jul 23, 2024
@LuohuaRain
Copy link
Contributor Author

Hi, @dvoituron
I do have an issue with FluentSlider, binding is only triggered first time to set initial value as Vincent said.
And I added the FluentSlider_Binding test, and it passed.

@vnbaaij vnbaaij enabled auto-merge (squash) July 23, 2024 17:09
@vnbaaij vnbaaij added this to the v4.9.4 milestone Jul 23, 2024
@vnbaaij vnbaaij merged commit 0f6d06a into microsoft:dev Jul 23, 2024
dannyldj pushed a commit to dannyldj/fluentui-blazor that referenced this pull request Sep 26, 2024
…mponent (microsoft#2438)

* [FluentSlider]: by using web components current-value instead of value

* fix: FluentSlider Tests

* chore: Tests.csproj

* Update Microsoft.FluentUI.AspNetCore.Components.Tests.csproj

* add FluentSlider_Binding test

---------

Co-authored-by: Vincent Baaij <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants