Skip to content

make dialog can be stack popup #86

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Coloryr
Copy link

@Coloryr Coloryr commented Mar 14, 2025

image
image

@SKProCH SKProCH self-requested a review March 14, 2025 09:01
@SKProCH
Copy link
Member

SKProCH commented Mar 16, 2025

I appreciate this work, the reason I haven't done it myself is because I don't know how it should be designed as a feature.

I need to spend a little more time thinking about this implementation.

Copy link
Member

@SKProCH SKProCH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall works fine, but i think there is need for proper handling of disabling of multiple dialogs. E.g. if i open 2 dialogs, i should be able to interact only with a top one (second one should be non interactable).
Close command also always closes the top dialog (but if other dialogs will be non interactable, then this is not a problem).

And probably we should move multidialog logic to separate control (since currently at least IsOpen would works weirdly), what do you think? Btw, after looking at this PR i kinda want to rewrite dialoghost almost from scratch, but i think what i don't have enough time

e.NameScope.Find<Rectangle>(ContentCoverName)?.AddDisposableHandler(PointerReleasedEvent, ContentCoverGrid_OnPointerReleased) ?? EmptyDisposable.Instance
};
base.OnApplyTemplate(e);
}

private class DisposeList : IDisposable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it should be moved into host itself. We actually doesn't want to control it from the host side

Copy link
Author

@Coloryr Coloryr Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is better

@SKProCH
Copy link
Member

SKProCH commented Mar 30, 2025

CurrentSession should be IReadOnlyList.
Also, you dont actually change anything about making non top dialogs non interactable.

E.g. if i open 2 dialogs, i should be able to interact only with a top one (second one should be non interactable).
Close command also always closes the top dialog (but if other dialogs will be non interactable, then this is not a problem).

Is it done intentionally, or you address this in a future, or i should do it myself?

@Coloryr
Copy link
Author

Coloryr commented Mar 31, 2025

CurrentSession should be IReadOnlyList. Also, you dont actually change anything about making non top dialogs non interactable.

E.g. if i open 2 dialogs, i should be able to interact only with a top one (second one should be non interactable).
Close command also always closes the top dialog (but if other dialogs will be non interactable, then this is not a problem).

Is it done intentionally, or you address this in a future, or i should do it myself?

CurrentSession would be better with the separate property, because CloseCommand only closes the current view, and turn next top view.
DisposeList It is used to unbind some bindings. When a top view closed, its should be canceled binding itself, or it can be move to pop view
I think it's better for you to complete it than for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants