Skip to content

Conversation

EricVernie
Copy link
Contributor

Related to Issue #263

To Test you need:

  1. An Office 365 Subscription
  2. Register an app in Azure Active Directory to get a Client Id

If you do not have any subscription, email me, I will send you a client id and test Account

Eric

@msftclas
Copy link

Hi @EricVernie, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@deltakosh deltakosh added this to the v1.1 milestone Aug 31, 2016
/// <summary>
/// Microsoft Graph Helper
/// </summary>
public class MicrosoftGraphHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

This class should be marked internal to hide it

@deltakosh
Copy link
Contributor

Seriously, this is utterly cool! Just some little tweaks and this will good to me

@msftclas
Copy link

msftclas commented Sep 1, 2016

@EricVernie, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

/// <summary>
/// Store the number of items
/// </summary>
private uint internalCount;
Copy link
Contributor

Choose a reason for hiding this comment

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

private fields should start with an underscore

uint maxItems)
{
this.func = func;
this._func = func;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed one :)

<Compile Include="Common\Constants.cs" />
<Compile Include="Common\DelegateCommand{T}.cs" />
<Compile Include="Common\SolidColorBrushConverter.cs" />
<Compile Include="Common\Models\SolidColorBrushConverter.cs" />
Copy link
Contributor

@deltakosh deltakosh Sep 6, 2016

Choose a reason for hiding this comment

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

Why moving these files? (Note that I'm not opposed to this, just want to understand :))

@deltakosh
Copy link
Contributor

All good to me, just need to see why moving files to /Common
beside that, it is LGTM

@hermitdave
Copy link
Contributor

@EricVernie there are some conflict.. would you like to get latest changes and push an update out ? I will review when you are done

@deltakosh
Copy link
Contributor

@hermitdave done :)

@deltakosh deltakosh merged commit 7fb6294 into CommunityToolkit:dev Sep 8, 2016
@deltakosh
Copy link
Contributor

I merged it...Please create issues if you find a problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants