-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: nested selection host support for IR-extension #618
base: main
Are you sure you want to change the base?
Conversation
fbdd764
to
79fd31a
Compare
79fd31a
to
efbf622
Compare
{ | ||
if (x is SelectorItem si) | ||
var host = GetUseNestedSelectionHost(ir) |
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.
Could we maybe avoid needing to have both a UseNestedSelectionHost
and a IsSelectionHost
property? Could we get away with just IsSelectionHost
?
Perhaps just search for a selection host if the itemRoot
itself is anything other than a SelectorItem
or ToggleButton
, otherwise assume the root should be the selection host? Or are there common scenarios that this logic wouldn't support?
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.
If we absolutely need both properties, perhaps we should bubble up some sort of warning/error when one is used without the other?
} | ||
private static void ApplyNestedTappedEventBlocker(ItemsRepeater ir, DependencyObject itemRoot) | ||
{ | ||
Console.WriteLine($"@xy droid:{IsAndroid}, wasm:{IsWasm}"); |
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.
leftover debug message?
@@ -33,14 +33,22 @@ public static int IndexOf(this ItemsSourceView isv, object item) | |||
} | |||
#endif | |||
|
|||
/// <summary> | |||
/// Update the selection indexes by toggling the provided index, and then coerced according to the selection mode. |
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.
TIL "indexes" is a thing and you're only supposed to use "indices" in the context of mathematical expressions
// Unlike for selection behaviors, we don't need to find the "selection host". | ||
// The selection host is a unrelated concept in the command setup. Additionally, | ||
// the template root would generally have the same context as the selection host. |
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.
// Unlike for selection behaviors, we don't need to find the "selection host". | |
// The selection host is a unrelated concept in the command setup. Additionally, | |
// the template root would generally have the same context as the selection host. | |
// Unlike for selection behaviors, we don't need to find the "selection host". | |
// The selection host is an unrelated concept in the command setup. Additionally, | |
// the template root would generally have the same context as the "selection host". |
GitHub Issue (If applicable): closes #611, re: #610
PR Type
What kind of change does this PR introduce?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements: