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

Fix ToggleSplitButton's IsCheckedProperty not being a public DP #2258

Conversation

Felix-Dev
Copy link
Contributor

Description

This PR adds a public DP IsCheckedProperty to the ToggleSplitButton.

Motivation and Context

Fixes #2218.

How Has This Been Tested?

Tested locally.

Open Questions

  1. Do I need to add an annotation for the added DP (like [WUXC_VERSION_PREVIEW])?
  2. Do we need to add a test here? I added an initial test to check if Binding works (no matching interaction test yet). It follows the enabled/disabled ToggleSplitButton test on the same MUXControlsTestApp page which also has no accompanying interaction test (1). Is it intentional that there is no interaction test for it?

(1)
togglesplitbutton

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Apr 8, 2020
@StephenLPeters
Copy link
Contributor

Technically, I think this would need API review... @MikeHillberg thoughts?

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters added the team-Controls Issue for the Controls team label Apr 8, 2020
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ranjeshj
Copy link
Contributor

@Felix-Dev thanks for fixing this. Can you add a quick api test in https://github.com/microsoft/microsoft-ui-xaml/blob/master/dev/SplitButton/APITests/SplitButtonTests.cs ?

You can just create the button, set the property, get it and check its value. You could also use SetValue/GetValue to validate.

@ranjeshj ranjeshj removed the needs-triage Issue needs to be triaged by the area owners label Apr 10, 2020
@Felix-Dev
Copy link
Contributor Author

@ranjeshj Added an API test. Should I remove the button I added to the MUXControlsTestApp (see image in opening post)?

@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ranjeshj
Copy link
Contributor

Technically, I think this would need API review... @MikeHillberg thoughts?

Following up on this. This should be a quick review, so no worries about it.

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

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

:shipit:

@StephenLPeters StephenLPeters merged commit 144d9bc into microsoft:master Apr 11, 2020
@Felix-Dev Felix-Dev deleted the user/Felix-Dev/togglesplitbutton-ischecked-binding branch April 11, 2020 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Make isChecked of ToggleSplitButton a bindable property
4 participants