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

[dev-v5] Add FluentSwitch component #3434

Open
wants to merge 41 commits into
base: dev-v5
Choose a base branch
from

Conversation

agriffard
Copy link
Contributor

@agriffard agriffard commented Feb 23, 2025

[dev-v5] Add FluentSwitch component

A FluentSwitch component represents a physical switch that allows someone to choose between two mutually exclusive options.
For example, "On/Off" and "Show/Hide". Choosing an option should produce an immediate result.

image

Best practices

Layout

When people need to perform extra steps for changes to take effect, use a check box instead. For example, if they must click a "Submit", "Next", or "OK" button to apply changes, use a check box.

Content

Only replace the On/Off labels if there are more specific labels for the setting. For example, you might use Show/Hide if the setting is "Show images".
Keep descriptive text short and concise—two to four words; preferably nouns. For example, "Focused inbox" or "WiFi".

@agriffard
Copy link
Contributor Author

Note: The LabelPosition example has a strange behaviour : The switches bound to the same value lose their synchronization, depending on which one is clicked.

Enregistrement.2025-02-23.154548.mp4

@dvoituron
Copy link
Collaborator

Indeed. You need to find why. 😀

@dvoituron dvoituron changed the title Switch v5 [dev-v5] Add FluentSwitch component Feb 23, 2025
@dvoituron
Copy link
Collaborator

  1. Should we keep those parameters? : ChildContent, CheckedMessage, UncheckedMessage
  2. If not, should we add a corresponding Removed properties section in the Migration page ?
  3. Creatin Unit tests with [Fact] attribute, only the .received.razor.html files were generated (not the .verified.razor.html). Is that the expected behaviour?
  1. I think these attributes could be removed from the new component.
  2. But to keep a compatibility, you could create another partial class in the Migration folder with these 3 parameters flagged as Obsolete and include it in the component: updat the Label property with CheckedMessage or UncheckedMessage if the Label is initially null and if the LabelPosition is only After. You must document all of this on the Migration page.
  3. Yes, that is the expected behavior: you need to create the .verified file. And it will be compared with the .received file. See the [documentation$https://github.com/microsoft/fluentui-blazor/blob/dev-v5/docs/unit-tests.md#fluentui-blazor-unit-tests)

Last "detail", in the PR description, can you include a summary of the component documentation (e.g. this PR #3267)?

@agriffard
Copy link
Contributor Author

Indeed. You need to find why. 😀

Same behaviour with 3 Checkboxes bound to @value1 (Only two components are enough to show this de-synchronization effect).

The html of all components is corresponding to the state of the @value1 (checked added or removed) but the components are not rendered accordingly.

@agriffard
Copy link
Contributor Author

agriffard commented Feb 24, 2025

  1. I think these attributes could be removed from the new component.
  2. But to keep a compatibility, you could create another partial class in the Migration folder with these 3 parameters flagged as Obsolete and include it in the component: updat the Label property with CheckedMessage or UncheckedMessage if the Label is initially null and if the LabelPosition is only After. You must document all of this on the Migration page.

[Obsolete] properties added in partial class and FluentSwitch added to Migration.

I could use something like this in the field for the Label:
Label="@(!string.IsNullOrEmpty(CheckedMessage) && CurrentValue ? CheckedMessage : (!string.IsNullOrEmpty(UncheckedMessage) && !CurrentValue ? UncheckedMessage : Label))"
but as the Properties are Obsolete, it doesn't compile when I use them.

@@ -0,0 +1,3 @@
fluent-switch[readonly] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you checked that the ReadOnly parameter blocks the use of the mouse and keyboard?

I think you need to apply this CSS style (on the “field” element and not on the “switch” element):

fluent-field:has(fluent-switch[readonly]) {
  pointer-events: none;
}

And to block the keyboard, you need to set the tabindex on the fluent-switch (like done) but also on the FluentField component. If not, using Tab the user can navigate to the component and press Space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, css added for readonly and tabIndex on FluentField.

---

### Removed values💥
The `ChildContent` property has been removed. Use `FluentField` instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you replace with

The ChildContent property has been removed. Use Label parameter instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, documentation for migration updated.

@attributes="AdditionalAttributes"
@onchange="@OnSwitchChangedHandler"
slot="input"
tabindex="@GetTabIndexValue" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few days ago, we added a new GetValueIfNoAdditionalAttribute extension method to simplify this “feature”. Can you use code like this?

tabindex="@AdditionalAttributes.GetValueIfNoAdditionalAttribute("tabindex", "-1", when: () => ReadOnly)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, AdditionalAttributes.GetValueIfNoAdditionalAttribute used for tabindex.

/// Gets or sets the checked message
/// </summary>
[Parameter]
//[Obsolete("This property is not supported anymore and will be removed in a future release.")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you uncomment these Obsolete attribute?

Copy link
Contributor Author

@agriffard agriffard Mar 6, 2025

Choose a reason for hiding this comment

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

If there is an [Obsolete], it doesn't compile anymore, with this message:

'FluentSwitch.UncheckedMessage' is obsolete: 'This property is not supported anymore and will be removed in a future release.'

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to add a pragma condition. We already used this in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thank you, Obsolete uncommented.


@code
{
public FluentSwitchTests()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Obsolete attributes (ChildContent, CheckedMessage and UncheckedMessage) must be unit tested.

You can run the file _StartCodeCoverage.cmd to validate if all code lines a covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, Tests added for ChildContent, CheckedMessage and UncheckedMessage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More tests added and _StartCodeCoverage.cmd run. 100% on branch coverage and a bit like FluentCheckBox on Line coverage.

}

[Fact]
public void FluentCheckbox_LabelPosition()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test can be omitted: you'll already be testing this position on the next next.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, FluentCheckbox_LabelPosition single test removed.

}

[Fact]
public void FluentCheckbox_LabelTemplate()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't test the LabelTemplate but the Label parameter.
(but this test in already include in FluentField tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, FluentCheckbox_LabelTemplate Test done properly.

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