Skip to content

added Command and CommandParameter on CheckBox#20717

Closed
pictos wants to merge 11 commits into
dotnet:net9.0from
pictos:pj/checkbox-command
Closed

added Command and CommandParameter on CheckBox#20717
pictos wants to merge 11 commits into
dotnet:net9.0from
pictos:pj/checkbox-command

Conversation

@pictos

@pictos pictos commented Feb 20, 2024

Copy link
Copy Markdown
Contributor

Description of Change

Issues Fixed

Fixes #7394

@ghost ghost added the community ✨ Community Contribution label Feb 20, 2024
@ghost

ghost commented Feb 20, 2024

Copy link
Copy Markdown

Hey there @pictos! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Feb 20, 2024
@pictos pictos marked this pull request as ready for review February 21, 2024 04:14
@pictos pictos requested a review from a team as a code owner February 21, 2024 04:14
@pictos pictos force-pushed the pj/checkbox-command branch from f53de5d to 29f9a43 Compare February 21, 2024 04:16
Comment thread src/Controls/tests/Core.UnitTests/CarouselViewTests.cs Outdated
Comment thread src/Controls/src/Core/CheckBox/CheckBox.cs

@rmarinho rmarinho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we also add a simple UITest to show how it renderers check and uncheck and a Visual test.

Thanks

Comment thread src/Controls/src/Core/CheckBox/CheckBox.cs Outdated
Comment thread src/Controls/src/Core/CheckBox/CheckBox.cs
@pictos

pictos commented Feb 23, 2024

Copy link
Copy Markdown
Contributor Author

@rmarinho does maui use inline docs normally or there's some xml file that I should add?

@pictos

pictos commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Could we also add a simple UITest to show how it renderers check and uncheck and a Visual test.

@rmarinho can you point me to a UITest that I can use as sample... I tried to look into the existing one on Appium project, but couldn't figure out how to create the UI to test

@rmarinho

rmarinho commented Feb 29, 2024

Copy link
Copy Markdown
Member

@rmarinho does maui use inline docs normally or there's some xml file that I should add?

We use inline docs now. @jfversluis can help you with that if you need.

Could we also add a simple UITest to show how it renderers check and uncheck and a Visual test.

@rmarinho can you point me to a UITest that I can use as sample... I tried to look into the existing one on Appium project, but couldn't figure out how to create the UI to test

Add your UI here :
https://github.com/dotnet/maui/blob/main/src/Controls/samples/Controls.Sample.UITests/Issues/Issue19379.xaml#L2

Add your test here:
https://github.com/dotnet/maui/blob/main/src/Controls/tests/UITests/Tests/Issues/Issue19379.cs#L27

Run it one time on CI to generate the screenshots and add them here:
https://github.com/dotnet/maui/tree/main/src/Controls/tests/UITests/snapshots

@jfversluis

Copy link
Copy Markdown
Member

does maui use inline docs normally or there's some xml file that I should add?

Yes please add the documentation inline and NOT the external XML files :)

@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024

@mattleibow mattleibow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The is looking close, but I had some questions/comments.

  • Should the command and the event always fire (or not) together?
  • Please add inline XML docs
  • Please add some unit tests for the checkbox in the areas:
    • CheckChanged event
    • Command/CommandParameter
    • IsEnabled
  • Please add some UI tests for checkbox:
    • Appears correctly in the correct state with Command CanExecute
    • Tapping the check box when enabled and disabled does the right thing

Comment on lines +27 to +31
checkBox.CheckedChanged?.Invoke(bindable, new CheckedChangedEventArgs((bool)newValue));
if (checkBox.Command is not null && checkBox.Command.CanExecute(checkBox.CommandParameter))
{
checkBox.Command.Execute(checkBox.CommandParameter);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure what we should be doing here as we are going to fire the CheckChanged event but NOT the command if the command disabled it.

The command can disable the checkbox, and that is fine. But, if the dev then manually sets checked and we only fire one, will that be unexpected? Do we have a place where we have an event and command and we have to do something like this?

I found Button does this:

public static void ElementClicked(VisualElement visualElement, IButtonElement ButtonElementManager)
{
	if (visualElement.IsEnabled == true)
	{
		ButtonElementManager.Command?.Execute(ButtonElementManager.CommandParameter);
		ButtonElementManager.PropagateUpClicked(); // <- this is the clicked event
	}
}

@mattleibow

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho

Copy link
Copy Markdown
Member

/rebase

@rmarinho

rmarinho commented Sep 9, 2024

Copy link
Copy Markdown
Member

Needs manual rebase

@rmarinho

Copy link
Copy Markdown
Member

Please reopen targeting the net10 branch

@rmarinho rmarinho closed this Jan 29, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-controls-checkbox CheckBox community ✨ Community Contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants