-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): make ProxyViewContainer.hidden hide child views (#10912) #10913
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
base: main
Are you sure you want to change the base?
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx test apps-automated -c=android |
❌ Failed | 9s | View ↗ |
nx run-many --target=test --configuration=ci --... |
✅ Succeeded | 3s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-05 19:14:37 UTC
|
|
|
We have hidden as well. The changes look accurate, had to adjust few things but it just ensures when using it on proxy that it’s children we marked hidden since proxy is not a native view itself. |
That's my bad, thanks for the correction @NathanWalker! |
|
@KL2400040448 @NathanWalker Instead of meddling with the property like this, maybe we can add an event listener for it. function onHiddenPropertyChange(args: EventData) {
const view = args.object as ProxyViewContainer;
view.eachChildView((child) => {
child.hidden = view.hidden;
return true;
});
}
@CSSType('ProxyViewContainer')
export class ProxyViewContainer extends LayoutBase {
private proxiedLayoutProperties = new Set<string>();
constructor() {
super();
this.nativeViewProtected = undefined;
this.on('hiddenProperty', onHiddenPropertyChange);
}
...
}This isn't as hackish as current solution and doesn't need ignore comments. |
KL2400040448
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.
approve to merge
6e2b60a to
73fc52c
Compare
|
✅ Rebased and resolved merge conflict. |
|
Hi @NathanWalker 👋 |
|
Hi @NathanWalker 👋 |
|
|
||
| /* | ||
| * Register/unregister existing children with the parent layout. | ||
| */ |
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.
Curious why valid comments removed?
|
|
||
| /** | ||
| * Layout property changed, proxy the new value to the child view(s) | ||
| */ |
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.
Same here.
|
|
||
| /** | ||
| * Apply the layout property to the child view. | ||
| */ |
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.
Another here.
| 'colSpan', | ||
| 'row', | ||
| 'rowSpan', | ||
| ]; |
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.
Same deal here. comments within are valid context.
| * removing children, so that they can update private measure data. | ||
| * | ||
| * We register our children with the parent to avoid breakage. | ||
| */ |
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.
Same here.
This PR fixes an issue where setting ProxyViewContainer.hidden = true did not hide its child views.
Problem
ProxyViewContainer is a virtual container that does not create a native view.
As a result, toggling its hidden property had no visible effect — its child views remained visible.
Solution
Override the hidden property to propagate the hidden state to all child views:
public set hidden(value: boolean) {
super.hidden = value;
this.eachChildView((child) => {
child.hidden = value;
return true;
});
}