Skip to content

Uno Platform Compatibility updates #3651

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

Merged
4 commits merged into from
Jan 7, 2021

Conversation

jeromelaban
Copy link
Contributor

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

What is the new behavior?

This PR adds a set of forward compatibilty changes to ease the cross-compilation of the community toolkit to Uno Platform targets.

  • Partials are needed for code generation
  • Use TargetFrameworks instead of TargetFramework to allow for late modification of the platform list

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Jan 5, 2021

Thanks jeromelaban for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi, Kyaa-dost and Rosuavio January 5, 2021 15:26
@jeromelaban jeromelaban force-pushed the dev/jela/uno-prep branch 3 times, most recently from e33b079 to 79cd819 Compare January 5, 2021 15:36
This change enables Uno Platform compatibility forward compatibility.
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

LGTM. FYI @Sergio0694 for the new Animations stuff and @RosarioPulella for the new Controls split.

@michael-hawker
Copy link
Member

@jeromelaban any reason this was opened as a draft in particular?

@michael-hawker michael-hawker added this to the 7.0 milestone Jan 6, 2021
@ghost ghost added the in progress 🚧 label Jan 6, 2021
@jeromelaban jeromelaban marked this pull request as ready for review January 6, 2021 00:42
@jeromelaban
Copy link
Contributor Author

@jeromelaban any reason this was opened as a draft in particular?

It was until I got the build passing :) It now is. Thanks for the review!

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Looks good to me 😄
Great to see some work towards unifying the codebase with Uno Platform! 🎉

Question: do we want to hold this off until #3639 is merged, since that completely rewrites the Microsoft.Toolkit.Uwp.UI.Animations package and hence would require the same changes applied to it too, or do we just want to merge this in the meantime and then manually do the same tweaks in there? 🤔

@jeromelaban
Copy link
Contributor Author

We're happy to contribute and make the toolkit even greater 🙂

This PR mainly reduces the number of partials to be merged, and it is expected that it'll be needed to add some more over time when Uno will make more steps toward being mergeable.

There is no need to consider adding partials blindly at this point, as far as the current master is concerned.

@michael-hawker
Copy link
Member

@Sergio0694 think we can just merge this now? We'll have a few things to sort out but merge conflicts should hopefully be pretty self-explanatory?

@jeromelaban
Copy link
Contributor Author

Thanks @RosarioPulella for the update, I indeed missed this one ;)

@Rosuavio
Copy link
Contributor

Rosuavio commented Jan 7, 2021

@jeromelaban, I noticed this in progress tag on this PR, is it ready?

@jeromelaban
Copy link
Contributor Author

@RosarioPulella it's not a tag I can manage, it was added automatically by the msftbot. This is PR is ready from my side.

@ghost
Copy link

ghost commented Jan 7, 2021

Hello @RosarioPulella!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 5fe63f4 into CommunityToolkit:master Jan 7, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants