Skip to content
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

[Fabric] Modal opens a new window on visible #12710

Merged
merged 21 commits into from
Mar 1, 2024

Conversation

TatianaKapos
Copy link
Contributor

@TatianaKapos TatianaKapos commented Feb 9, 2024

Description

When visible is set to true, Modal creates a new window with the root hwnd as the parent window.

Type of Change

  • New feature (non-breaking change which adds functionality)

Why

Currently Modal does nothing, now Modal does something! Incrementally adding small changes to Modal but looking for feedback if this is the right direction we want to go in :)

Next Steps for Modal documented here: #11157. Next step is to add react native components within the new window

What

Creates and register a new window for Modal. Currently you can only destroy the window by manually pressing the 'X' button as opposed to setting visible to false.

Screenshots

Playground-composition_xs1HVgun38.mp4

Testing

tested locally

Changelog

no

Microsoft Reviewers: Open in CodeFlow

@TatianaKapos TatianaKapos requested review from a team as code owners February 9, 2024 19:14
Comment on lines +159 to +160
WINRT_VERIFY(classId); // 0 = fail
winrt::check_win32(!classId);
Copy link
Member

Choose a reason for hiding this comment

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

Is WINRT_VERIFY a debug-only assert? looks like no
Why do we have both checks here?

check_win32 takes in a... success code? reference. But this an ATOM. Doesn't look like it does the work of calling GetLastError? Basically... I'm not sure this is the right error checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh interesting! I'm not sure on the right error checking for an ATOM but I can do some research! (unless someone else knows)

Doing a quick search of our repo, https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/microsoft/react-native-windows%24+RegisterClassEx&patternType=keyword&sm=0, it looks like this is how we check for an error when calling RegisterClassEX in multiple place, wondering if we will also want to change these?

const int MODAL_DEFAULT_HEIGHT = 500;

// creates a new modal window
void WindowsModalHostComponentView::ShowOnUIThread() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you refactor this slightly into two functions, something like:

  1. EnsureModalCreated() which does the work of checking m_hwnd and building the window
  2. ShowOnUIThread() which calls that function then calls ShowWindow, etc.

I think we'll run into other places where we'll want to create the window so it's ready for UI, even if we don't show it right away.

@TatianaKapos TatianaKapos merged commit 5458f11 into microsoft:main Mar 1, 2024
48 checks passed
@TatianaKapos TatianaKapos deleted the tk-modal branch March 1, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants