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

Add VB samples to docs #1977

Merged
merged 16 commits into from
Apr 30, 2018
Merged

Conversation

mrlacey
Copy link
Contributor

@mrlacey mrlacey commented Apr 15, 2018

Issue: #1865

PR Type

What kind of change does this PR introduce?

  • Documentation content changes

What is the current behavior?

Code samples are C# only

What is the new behavior?

Code samples for the website are now in C# and VB.Net

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • 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

I've separately tested with @nmetulev that these changes will create the behavior on the website where a person can choose their preferred language from a menu on the side of the screen and then all samples will switch to that language.

Changes to the code included in the sample app will follow in a separate PR.

@nmetulev
Copy link
Contributor

ping @Vijay-Nirmal for review

Copy link
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

Tested on docs and works as expected:
image

Tested in sample app and works as expected.

Other than the comments below, looks amazing. And thank you for fixing few code blocks and so many typos :)

@@ -35,6 +38,12 @@ anim.SetDurationForAll(2500);
anim.SetDelay(250);
anim.Start();
```
```vb
Copy link
Contributor

@nmetulev nmetulev Apr 15, 2018

Choose a reason for hiding this comment

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

The XAML and C# titles should be removed since each code block already specifies the language.

image

@Vijay-Nirmal, should this be done in your PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

On it

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

' Open a connection with the stream service in order to receive live tweets and events
ListView.ItemsSource = _tweets
Await TwitterService.Instance.StartUserStreamAsync(Async Sub(tweet)
Await Dispatcher.RunAsync(CoreDispatcherPriority.Normal, Sub()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this normal to be indented like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VS auto indents like this. If it's unreadable on the docs site I can try and reformat.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how it looks like on docs:
image

It's readable, but not ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix submitted

Copy link
Contributor

@Vijay-Nirmal Vijay-Nirmal 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. Update .template.md file.

This PR should be open untill #1748 because that PR has some C# code.

@nmetulev
Copy link
Contributor

Now that #1748 is merged, I think we can proceed with this PR. @mrlacey, once you resolve conflicts we should be good to go to merge.

…VBSamplesToDocs

# Conflicts:
#	docs/animations/AnimationSet.md
#	docs/animations/FadeHeader.md
#	docs/animations/Saturation.md
#	docs/controls/BladeView.md
#	docs/controls/Carousel.md
#	docs/controls/HamburgerMenu.md
#	docs/controls/InAppNotification.md
#	docs/controls/RangeSelector.md
#	docs/helpers/AdvancedCollectionView.md
#	docs/helpers/BackgroundTaskHelper.md
#	docs/helpers/Colors.md
#	docs/helpers/DeepLinkParsers.md
#	docs/helpers/HttpHelper.md
#	docs/helpers/HttpHelperRequest.md
#	docs/helpers/HttpHelperResponse.md
#	docs/helpers/PrintHelper.md
#	docs/helpers/StorageFiles.md
#	docs/helpers/Streams.md
#	docs/services/Bing.md
#	docs/services/Facebook.md
#	docs/services/Linkedin.md
#	docs/services/MicrosoftGraph.md
#	docs/services/OneDrive.md
#	docs/services/Twitter.md
@mrlacey
Copy link
Contributor Author

mrlacey commented Apr 27, 2018

@nmetulev merge issues addressed and I've added VB versions of the new C# samples too

@Vijay-Nirmal
Copy link
Contributor

@mrlacey Update .template.md file with the info to add VB codes.

@nmetulev
Copy link
Contributor

Looks good. Agree with @Vijay-Nirmal, should update the template with guidance around adding VB code, can you add it @mrlacey ?

Plus, fixed a couple of other typos
@mrlacey
Copy link
Contributor Author

mrlacey commented Apr 30, 2018

Looks good. Agree with @Vijay-Nirmal, should update the template with guidance around adding VB code, can you add it @mrlacey ?

I've updated the template as I feel is appropriate.
In WTS we make including VB support/docs optional as it's seen as a barrier to contribution from devs who only do C#. We use a script/manual process to identify gaps and inconsistencies in the different language support and address those separately. Not sure how you want to handle this in the toolkit though.

@nmetulev
Copy link
Contributor

Good point. How about we add a comment above the VB code block specifying that it is optional and ask the developer to add the dev lang at the top if they include VB?

@mrlacey
Copy link
Contributor Author

mrlacey commented Apr 30, 2018

@nmetulev the comment I added to the template was

<!-- If you only include syntax for a single language, update the language list at the top of the file too -->

@nmetulev
Copy link
Contributor

nmetulev commented Apr 30, 2018

If we want to make it more optional, we should ask the developer to add the - vb instead of asking everyone to remove it, what do you think?

@mrlacey
Copy link
Contributor Author

mrlacey commented Apr 30, 2018

@nmetulev I've updated the template to make VB samples optional. Is this more in line with what you were thinking?

@nmetulev nmetulev merged commit c53293a into CommunityToolkit:master Apr 30, 2018
@windowstoolkitbot
Copy link

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1865

@nmetulev
Copy link
Contributor

Perfect, thank you, merged :)

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