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

doing feedback from SSO PM #556

Merged
merged 3 commits into from Nov 13, 2018
Merged

doing feedback from SSO PM #556

merged 3 commits into from Nov 13, 2018

Conversation

Rick-Kirkham
Copy link
Contributor

No description provided.

@o365devx
Copy link

✅ Validation status: passed

File Status Preview URL Details
docs/develop/create-sso-office-add-ins-nodejs.md ✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@@ -551,7 +552,9 @@ There are two server-side files that need to be modified.
* The MSGraphHelper class is defined in src\msgraph-helper.ts.
* We minimize the data that must be returned by specifying that we only want the name property and only the first 3 items.

`const graphData = await MSGraphHelper.getGraphData(graphToken, "/me/drive/root/children", "?$select=name&$top=3");`
```javascript
const graphData = await MSGraphHelper.getGraphData(graphToken, "/me/drive/root/children", "?$select=name&$top=3");
Copy link
Collaborator

Choose a reason for hiding this comment

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

With const and await, isn't this TypeScript?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is a TypeScript walkthrough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then the code type should be "typescript" instead of "javascript" :-)

@@ -33,6 +33,7 @@ This article walks you through the process of enabling single sign-on (SSO) in a
> * The **Before** folder is a starter project. The UI and other aspects of the add-in that are not directly connected to SSO or authorization are already done. Later sections of this article walk you through the process of completing it.
> * The **Completed** version of the sample is just like the add-in that you would have if you completed the procedures of this article, except that the completed project has code comments that would be redundant with the text of this article. To use the completed version, just follow the instructions in this article, but replace "Before" with "Completed" and skip the sections **Code the client side** and **Code the server** side.
> * The **Completed Multitenant** version is a completed sample that supports multitenancy. Explore this sample if you intend to support Microsoft accounts from different domains with SSO.
> *Regardless of which version you use, you will need to trust a cert for the localhost. See the "IMPORTANT" note in the Readme of the repo.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: "certificate" instead of "cert"
(I'm not actually sure...)

@@ -33,6 +33,7 @@ This article walks you through the process of enabling single sign-on (SSO) in a
> * The **Before** folder is a starter project. The UI and other aspects of the add-in that are not directly connected to SSO or authorization are already done. Later sections of this article walk you through the process of completing it.
> * The **Completed** version of the sample is just like the add-in that you would have if you completed the procedures of this article, except that the completed project has code comments that would be redundant with the text of this article. To use the completed version, just follow the instructions in this article, but replace "Before" with "Completed" and skip the sections **Code the client side** and **Code the server** side.
> * The **Completed Multitenant** version is a completed sample that supports multitenancy. Explore this sample if you intend to support Microsoft accounts from different domains with SSO.
> *Regardless of which version you use, you will need to trust a cert for the localhost. See the "IMPORTANT" note in the Readme of the repo.*
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 meant to be another bullet point or an extension of the 'Completed Multitenant' item? Current behavior is that it's an italicized sentence in the 'Completed Multitenant' bullet point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's meant to be a completely separate paragraph after the bulleted list. Fixed now.

@o365devx
Copy link

✅ Validation status: passed

File Status Preview URL Details
docs/develop/create-sso-office-add-ins-nodejs.md ✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@@ -551,7 +552,9 @@ There are two server-side files that need to be modified.
* The MSGraphHelper class is defined in src\msgraph-helper.ts.
* We minimize the data that must be returned by specifying that we only want the name property and only the first 3 items.

`const graphData = await MSGraphHelper.getGraphData(graphToken, "/me/drive/root/children", "?$select=name&$top=3");`
```javascript
const graphData = await MSGraphHelper.getGraphData(graphToken, "/me/drive/root/children", "?$select=name&$top=3");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then the code type should be "typescript" instead of "javascript" :-)

@o365devx
Copy link

✅ Validation status: passed

File Status Preview URL Details
docs/develop/create-sso-office-add-ins-nodejs.md ✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@Rick-Kirkham Rick-Kirkham merged commit 716be1b into master Nov 13, 2018
@kbrandl kbrandl deleted the miscellaneous-sso-updates branch November 14, 2018 20:57
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.

None yet

4 participants