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

Code Quality: Use WindowContext instead of AppModel #15302

Closed
wants to merge 9 commits into from

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented May 4, 2024

AppModel was leftover from UWP, removed and replaced with WindowContext which can be DI'd.

@yaira2 yaira2 added the needs-testing Pull request requires testing before being merged label May 5, 2024
@yaira2
Copy link
Member

yaira2 commented May 5, 2024

For testing, we should test one or two of the properties and ensure they are updated correctly. For example, we should ensure that IsCompactOverlay and IsPasteEnabled are correctly updated when the state changes.

@yaira2 yaira2 added changes requested Changes are needed for this pull request and removed needs-code review labels May 5, 2024
@0x5bfa
Copy link
Member Author

0x5bfa commented May 6, 2024

Sorry for making extra change while you finished review.
I wanted to make sure reduce App.WindowContext reference.

@0x5bfa
Copy link
Member Author

0x5bfa commented May 7, 2024

Now, ready to be merged

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed changes requested Changes are needed for this pull request labels May 7, 2024
yaira2
yaira2 previously approved these changes May 7, 2024
@0x5bfa
Copy link
Member Author

0x5bfa commented May 7, 2024

I'm not sure why it's fail again and again since I can build, deploy and see that the window successfully opens without any crash.

It's failing here:

TestHelper.InvokeButtonById("InnerNavigationToolbarNewButton");

Can you check if you can open New... button on your end?

@yaira2 yaira2 dismissed their stale review May 7, 2024 20:07

The merge-base changed after approval.

@yaira2
Copy link
Member

yaira2 commented May 7, 2024

Try rebasing the branch from main.

@yaira2 yaira2 force-pushed the 5bfa/CQ-AppModelRemoval branch 3 times, most recently from e67d9fb to b32f21f Compare May 12, 2024 15:08
@yaira2
Copy link
Member

yaira2 commented May 12, 2024

@0x5bfa can you try forking these changes to another branch to see if that fixes the CI issues?

@0x5bfa
Copy link
Member Author

0x5bfa commented May 12, 2024

Sure.

@0x5bfa 0x5bfa closed this May 12, 2024
@0x5bfa 0x5bfa deleted the 5bfa/CQ-AppModelRemoval branch May 16, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-testing Pull request requires testing before being merged ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants