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.

Sign up for GitHub

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

Jump to bottom

finish xamarin getting started article and code sample #2497

Merged
merged 1 commit into from
Jul 10, 2020
Merged

finish xamarin getting started article and code sample #2497

merged 1 commit into from
Jul 10, 2020

Conversation

codemillmatt
Copy link
Contributor

No description provided.

@codemillmatt
Copy link
Contributor Author

@JeremyLikness for review

JeremyLikness
JeremyLikness suggested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@JeremyLikness JeremyLikness left a comment

Choose a reason for hiding this comment

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

Few questions:

  • Has this been tested in Release mode? Want to be sure it works without any special compiler tweaks
  • What is the startup time?

entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/toc.yml Outdated Show resolved Hide resolved
@codemillmatt
Copy link
Contributor Author

@JeremyLikness - per your comments. Yes the sample was tested in Release mode and it works (at least w/ all the default Release settings).

Start-up time ... it's not great. It you'd like a more quantitative answer, I'd need to mess around with it more.

@codemillmatt codemillmatt marked this pull request as ready for review July 7, 2020 23:46
JeremyLikness
JeremyLikness suggested changes Jul 8, 2020
View reviewed changes
Copy link
Member

@JeremyLikness JeremyLikness left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great to me! Only two other changes from my end:

  1. Change author: to be your GitHub id. Then add ms.author with your Microsoft alias.
  2. I think there is an issue with syntax highlighting, possibly due to the the "Main" in the "code-csharp" tags. Can you try taking the Main out so it looks like [!code-csharp[](...)] instead of [!code-csharp[Main](..)] ? That worked for me in a different PR.

@codemillmatt
Copy link
Contributor Author

@JeremyLikness changes are there.

JeremyLikness
JeremyLikness previously requested changes Jul 8, 2020
View reviewed changes
Copy link
Member

@JeremyLikness JeremyLikness left a comment

Choose a reason for hiding this comment

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

So close! Just a small tweak and then I'm good.

entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
smitpatel
smitpatel reviewed Jul 8, 2020
View reviewed changes
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
entity-framework/core/get-started/xamarin.md Outdated Show resolved Hide resolved
@codemillmatt
Copy link
Contributor Author

So close! Just a small tweak and then I'm good.

Everything should be in there now @JeremyLikness

@codemillmatt
Copy link
Contributor Author

#sign-off

ajcvickers
ajcvickers reviewed Jul 10, 2020
View reviewed changes
samples/core/Xamarin/EFGetStarted.Android/EFGetStarted.Android.csproj
@@ -0,0 +1,106 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Does Xamarin require old-style projects like this?

ajcvickers
ajcvickers approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@ajcvickers
Copy link
Member

@smitpatel Are you okay to merge this? We can always iterate on it again when @JeremyLikness is back.

@smitpatel
Copy link
Member

LGTM from build perspective.

@codemillmatt - Please rebase on latest master and squash all commits in one.
If you are using github UI to merge the PR then please use "Squash and Merge" option in green big button.

@codemillmatt
finish xam article and code sample
cc02260 
updating toc

adding images and requested PR changes

updating code from outside files and metadata

fixing suggested changes
@codemillmatt
Copy link
Contributor Author

@smitpatel - rebased & squashed.

@smitpatel smitpatel dismissed JeremyLikness’s stale review July 10, 2020 20:13

Can review later if any changes needed.

@smitpatel smitpatel merged commit edc9c7f into dotnet:master Jul 10, 2020
@ajcvickers ajcvickers mentioned this pull request Jul 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajcvickers ajcvickers ajcvickers approved these changes

@bricelam bricelam Awaiting requested review from bricelam

@smitpatel smitpatel Awaiting requested review from smitpatel

@JeremyLikness JeremyLikness Awaiting requested review from JeremyLikness

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@codemillmatt @ajcvickers @smitpatel @JeremyLikness