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

[FluentSlider] [FluentNumberField] Fix #2948 #3077

Merged
merged 9 commits into from
Dec 27, 2024
Merged

Conversation

oneolddev
Copy link
Contributor

@oneolddev oneolddev commented Dec 21, 2024

Pull Request

📖 Description

Fixes issues reported in #2948. Two issue reported both related to changing the value of the control programatically.

🎫 Issues

The underlying issue is that the web components being wrapped were designed to have there states only changed interactively. bind-value does not behave as expected. Inward binding works only on for initialization. Outward binding works as expected.

This gives the appearance of a sync issue.

👩‍💻 Reviewer Notes

FluentSlider - #2665 did not fully fix #2609. The original fix was to prevent the flickering when the slider was moved rapidly originally reported. I was not able to reproduce this problem with the current implementation of FluentDataGrid. I did end up adding debouncing with the DebounceAction. I believe I did this correctly.

FluentNumberField - The fix was to ensure the underlying web component reflected the correct value. I could not find a way of distinguishing if the value was changed by manipulating the control or if it was changed programatically. I opted to use some javascript to force the value in the web component. I also added a DisposeAsync. It was missing.

I've also included an issue-tester page as part of this draft

📑 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

Copy link
Collaborator

@vnbaaij vnbaaij left a comment

Choose a reason for hiding this comment

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

Will test this soop

examples/Demo/Shared/Pages/Lab/IssueTesterPage.razor Outdated Show resolved Hide resolved
@oneolddev
Copy link
Contributor Author

@vnbaaij

I've fixed the unit test by easing the BUnit JSInterop mode. I'm not sure if that breaks any testing conventions.

Should this be split into two PRs? One for FluentSlider and one for FluentNumberField.

@oneolddev oneolddev marked this pull request as ready for review December 21, 2024 21:24
@oneolddev oneolddev requested a review from dvoituron as a code owner December 21, 2024 21:24
@oneolddev oneolddev marked this pull request as draft December 22, 2024 22:50
@oneolddev
Copy link
Contributor Author

oneolddev commented Dec 23, 2024

Found another issue with my fix for FluentNumberField 😞

When typing directly into the control, it would flicker and lose the change.

@oneolddev oneolddev marked this pull request as ready for review December 23, 2024 06:31
Copy link
Collaborator

@dvoituron dvoituron left a comment

Choose a reason for hiding this comment

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

Next time, please create one PR to fix one problem. This makes it easier to maintain the code.
Thank you :-)

@vnbaaij vnbaaij merged commit 1c28dbb into microsoft:dev Dec 27, 2024
4 checks passed
@oneolddev oneolddev deleted the fix-2948 branch December 31, 2024 02:19
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.

[Bug] FluentSlider two-way binding issue
3 participants