-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add basic SwitchPresenter Sample #3789
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
Add basic SwitchPresenter Sample #3789
Conversation
|
Thanks michael-hawker for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
Microsoft.Toolkit.Uwp.UI.Controls.Primitives/SwitchPresenter/SwitchPresenter.cs
Outdated
Show resolved
Hide resolved
Fix name of TargetType DependencyProperty Add better Enum string support
…ection or case values Can't think of scenarios where this was actually useful and added a lot of complexity. Removing it for now for the core value the control provides and simplifies the structure. Samples from Sample App continue to work as expected.
41225e1 to
2e3ccec
Compare
|
@chingucoding thanks for taking a look, I've made updates based on our discussions and simplified the logic as you suggested. @RosarioPulella or @Kyaa-dost want to take a quick look? This is ready to go now! 🎉🎉🎉 |
|
Hello @michael-hawker! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Primitives/SwitchPresenter.bind
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Primitives/SwitchPresenter.bind
Show resolved
Hide resolved
Sergio0694
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor questions, but looks good and works great! 🎉
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="CaseCollection"/> class. | ||
| /// </summary> | ||
| public CaseCollection() | ||
| { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this could safely be removed in theory, right?
| /// An collection of <see cref="Case"/> to help with XAML interop. | ||
| /// </summary> | ||
| public class CaseCollection : IList<Case>, IEnumerable<Case> // TODO: Do we need this or can we use an ObservableCollection directly??? (Or is it useful to have it manage the registration of the child events?) | ||
| public class CaseCollection : DependencyObjectCollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not sealed on purpose?
| static object ThrowExceptionForKeyNotFound() | ||
| { | ||
| throw new InvalidOperationException("The requested enum value was not present in the provided type."); | ||
| } | ||
|
|
||
| return ThrowExceptionForKeyNotFound(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, nice! 😄
| Cat, | ||
| Dog, | ||
| Bunny, | ||
| Llama, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most important change in this PR :P
| ui:TextBoxExtensions.Regex="^[a-zA-Z]{6}$" | ||
| Header="Confirmation code" | ||
| PlaceholderText="6 letters" /> | ||
| <TextBlock Visibility="{Binding (ui:TextBoxExtensions.IsValid), ElementName=ConfirmationCodeValidator}">Thanks for entering a valid code!</TextBlock> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use Text property for fast path rendering
| ui:TextBoxExtensions.Regex="(^\d{10}$)|(^\d{13}$)" | ||
| Header="E-ticket number" | ||
| PlaceholderText="10 or 13 numbers" /> | ||
| <TextBlock Visibility="{Binding (ui:TextBoxExtensions.IsValid), ElementName=TicketValidator}">Thanks for entering a valid code!</TextBlock> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same here
Also Fixes #3671, and related to #1908
Adds a basic sample for SwitchPresenter to the sample app. Would like to do a more advanced one still though. This also fixes a small bug in the case selection logic.
Any suggestions for sample scenarios where we could switch off a strong type like an enum or some other type would be appreciated. 🙂
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Would crash if no default case provided and no matching case available.
(And no sample.)
What is the new behavior?
Control goes back to presenting nothing if no match or default is available.
PR Checklist
Please check if your PR fulfills the following requirements:
TO DO