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

Ensure correct .props and .targets order in nuget package #7574

Merged
merged 4 commits into from
Aug 11, 2022

Conversation

krschau
Copy link
Contributor

@krschau krschau commented Aug 8, 2022

Fix an issue where WinUI 2.8 couldn't be used in XAML islands apps.

Description

When you add a nuget package reference, VS will add imports for the package’s props and targets. Props go at the top, targets at the bottom. WinUI didn't have a props file in the right location in the package (in build\native), only targets. The WinUI targets import a props file from a directory above. This means that it’s possible for WebView2’s targets to be imported before WinUI's targets (and therefore the WinUI props), rendering the project unbuildable. This change moves the props file to the build/native directory, and removes the unnecessary import.

http://task.ms/40798730

@krschau krschau requested review from asklar and kmahone August 8, 2022 19:46
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Aug 8, 2022
@krschau
Copy link
Contributor Author

krschau commented Aug 8, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@krschau
Copy link
Contributor Author

krschau commented Aug 9, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@asklar asklar left a comment

Choose a reason for hiding this comment

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

lgtm for release, might want to make sure this works with experimental/prerelease package too (you might need to do the same kind of update for the experimental props/targets)

@krschau krschau merged commit 871c27e into main Aug 11, 2022
@krschau krschau deleted the user/krschau/targets-props branch August 11, 2022 22:15
@ghost
Copy link

ghost commented Aug 15, 2022

🎉Microsoft.UI.Xaml v2.8.1 has been released which incorporates this pull request.:tada:

Handy links:

@ranjeshj ranjeshj added area-NugetPackage Issue with the nuget package developer experience (e.g. build error, missing files) team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Aug 26, 2022
zadjii-msft added a commit to microsoft/terminal that referenced this pull request Apr 3, 2023
DHowett added a commit to microsoft/terminal that referenced this pull request Apr 3, 2023
Updates the Terminal to Microsoft.UI.Xaml v2.8. 

* MUX 2.8 adds a dependency on WebView2, so we need to include parts of it too.
* See microsoft/microsoft-ui-xaml#7574 for why
we're adding the `.props`
* The TabView thing: 
> tl;dr: In >=MUX 2.7, we were updating our tab colors by doing a
"Visual State Dance", as I called it. We'd manually change the
`TabViewItem`'s VisualState to one that it wasn't in, then change it
back to the one it should be in. This seemingly re-applied the new
values of the brushes. However in 2.8, this seemingly didn't work
anymore!
  > 
  > So instead, we do a "Theme Dance", like so:
  > ```c++
  >   const auto& reqTheme = TabViewItem().RequestedTheme();
  >   TabViewItem().RequestedTheme(ElementTheme::Light);
  >   TabViewItem().RequestedTheme(ElementTheme::Dark);
  >   TabViewItem().RequestedTheme(reqTheme);
  >   ```
> This causes the `ThemeResource`s to be re-evaluated to the new values.
> We never got to the root cause of why this seems different in 2.8. It
literally makes no sense.
Closes #13495

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NugetPackage Issue with the nuget package developer experience (e.g. build error, missing files) team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants