-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Applying visibility change to child controls #20154
base: main
Are you sure you want to change the base?
Applying visibility change to child controls #20154
Conversation
Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
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.
@tj-devel709 Could we include a device or ui tests here?
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.
This will need more description about the change and maybe some discussion first. I m not sure we want to add this to our public API.
Also this would need device and unit tests for the new api
@rmarinho there was a short discussion in this issue #19139 I will write tests once there is a decision on whether this change makes sense or not. |
I added this PR here to address specifically with the next entry capability. I don't think we will need this PR (in regards to the next entry issue), but it could possibly be helpful for a different scenario! |
This PR seems sensible, but both WPF and UWP chose not to propagate this value. So I am asking some teams why they decided this: dotnet/wpf#8774 |
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.
Looking good, we just need a bunch more tests for a good few combinations so we can ensure we don't break somerthing later on and all things stay working as expected.
Can you rebase? |
90d5d80
to
1b85ac4
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Now the question is... Is this a breaking change for a SR that means it should wait for net9.0? Or is it OK and people need to be aware of the change? |
/// This method must always be called if some event occurs and the value of | ||
/// the <see cref="IsVisibleCore"/> property will change. | ||
/// </summary> | ||
protected void RefreshIsVisibleProperty() => |
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.
For a service release, maybe this should be protected internal
for now. In net9.0 we can make it public.
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 think it is a good idea
b4370b4
to
549cd74
Compare
@kubaflo Could you rebase and fix the conflicts?. Thanks. |
549cd74
to
5de6439
Compare
@jsuarezruiz done :) |
5de6439
to
cd85b97
Compare
cd85b97
to
6cafb41
Compare
Uff needs rebase again |
6cafb41
to
67fee02
Compare
@rmarinho rebased |
A workaround for this issue might be to subscribe It would be great to know if an element is visible or not though. |
218ddbf
to
33f73ee
Compare
This PR should target |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Description of Change
Applying visibility change to child controls
Issues Fixed
Fixes #19139
Screen.Recording.2024-01-25.at.13.28.42.mov