Skip to content

Conversation

@pictos
Copy link
Contributor

@pictos pictos commented Feb 20, 2024

Description of Change

Issues Fixed

Fixes #7394

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

ghost commented Feb 20, 2024

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
Copy link
Member

@rmarinho rmarinho left a comment

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

@pictos
Copy link
Contributor Author

pictos commented Feb 23, 2024

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

@pictos
Copy link
Contributor Author

pictos commented Feb 26, 2024

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
Copy link
Member

rmarinho commented Feb 29, 2024

@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
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
Copy link
Member

@mattleibow mattleibow left a comment

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
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
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

rmarinho commented Sep 9, 2024

Needs manual rebase

@rmarinho
Copy link
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