-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
vth #1820
vth #1820
Conversation
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public interface IVisualTreeHelper | ||
{ | ||
IReadOnlyList<object> GetVisualChildren(); |
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.
object
might be a bit too broad. You can use IView
, or IElement
if you need to include windows as "visual children".
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.
I agree. Since everything is based on IElement
and that's the base, we should be safe using that instead of object
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.
IElement is internal, are you ok with the interface being internal too @drasticactions ? also, TableSection isn't an Element, but you used to return it in the other implementation...
@drasticactions please change this PR the way you prefer it, that'd be easier
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.
IElement is public. The core one that is. The one in controls is not the same and is not the correct one
@@ -437,5 +437,7 @@ protected internal virtual void CleanUp() | |||
|
|||
NavigationProxy = null; | |||
} | |||
|
|||
IReadOnlyList<object> IVisualTreeHelper.GetVisualChildren() => new List<object> { MainPage }.AsReadOnly(); |
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.
Should this be the Windows instead? MainPage is more of a helper method and will always create a window..
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.
the purpose of this PR is to help the XET team deliver their features. It should be whatever they need 🤷♂️
@@ -638,5 +638,7 @@ private protected virtual void OnParentChangingCore(Element oldParent, Element n | |||
ParentChanging?.Invoke(this, args); | |||
OnParentChanging(args); | |||
} | |||
|
|||
IReadOnlyList<object> IVisualTreeHelper.GetVisualChildren() => LogicalChildren; |
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.
@hartez is this still LogicalChildren? I recall a PR somewhere that did something with this property. Not sure if it was additive or now is a bigger change.
Superseded by #1845 |
Description of Change
Implements #
Additions made
PR Checklist
Does this PR touch anything that might affect accessibility?
If any of the above checkboxes apply to your PR, then the PR will need to provide testing to demonstrate that accessibility still works.