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

New command Add Client Side WebPart to Modern Page #502

Closed
wants to merge 1 commit into from
Closed

New command Add Client Side WebPart to Modern Page #502

wants to merge 1 commit into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented Jun 25, 2018

[EDIT] I forgot to mention the related issue #366

@coveralls
Copy link

coveralls commented Jun 25, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 8042091 on ypcode:dev-addclientsidewebpart into 43900ec on pnp:dev.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jun 25, 2018

Thanks @ypcode, will have a look shortly

@waldekmastykarz waldekmastykarz self-assigned this Jun 25, 2018
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Hey @ypcode, would you mind doing a few adjustments? Thanks!

ClientSidePageComponent,
CanvasColumn
} from './clientsidepages';
import { StandardWebPart, StandartWebPartUtils } from './StandardWebpartTypes';
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

Could you rename the StandardWebpartTypes file to StandardWebPartTypes for consistent spelling? 🙂

.then((csPage) => {
if (this.debug) {
cmd.log(`Retrieved Client Side Page:`);
cmd.log(csPage);
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

When running the command with the --debug option, the command fails at this line with the following error:

Retrieved Client Side Page:
Error: Converting circular structure to JSON

Since we're already including the retrieved page contents in the _getClientSidePage method, it's not necessary to do it again here, so this could be removed.

cmd.log('');
}

request.get(requestOptions).then((res) => {
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

If you specify a name of the page that doesn't exist, the CLI will be stuck here, because this promise doesn't handle, rejection. As a result, the parent promise is never completed.

)}/sitepages/${encodeURIComponent(pageName)}')?$expand=ListItemAllFields/ClientSideApplicationId`,
headers: Utils.getRequestHeaders({
authorization: `Bearer ${accessToken}`,
'content-type': 'application/json;charset=utf-8',
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

since this is a GET request, there is no need to specify content-type

url: `${webUrl}/_api/web/getclientsidewebparts()`,
headers: Utils.getRequestHeaders({
authorization: `Bearer ${accessToken}`,
'content-type': 'application/json;charset=utf-8',
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

since this is a GET request there is no need to specify the content-type

accessToken: string,
requestDigest: string
): request.RequestPromise {
let pageName: string = args.options.pageName;
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

We have the page name in the parent method already so why not pass it here as argument?

description: 'URL of the site where the page to retrieve is located'
},
{
option: '--standardWebPart <standardWebPart>',
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

standardWebPart is optional, so we can use [standardWebPart] instead of <standardWebPart>. Here it's just notation, but it's a common convention in command line tools that we follow in the CLI as well

| PageTitle | People | QuickLinks | CustomMessageRegion | Divider | MicrosoftForms | Spacer`
},
{
option: '--webPartId <webPartId>',
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

webPartId is optional so let's use [webPartId] instead of <webPartId>

}
}

if (args.options.section && args.options.section < 1) {
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

we should add validation to ensure that the specified value is a number

return 'The value of parameter section must be 1 or above';
}

if (args.options.column && args.options.column < 1) {
Copy link
Member

@waldekmastykarz waldekmastykarz Jul 6, 2018

Choose a reason for hiding this comment

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

we should add validation to ensure that the specified value is a number

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jul 6, 2018

Forgot to mention: the page-clientsidewebpart-add.md file, must be linked from the ./docs/manual/mkdocs.yml file or it won't be included in the build docs.

@ypcode
Copy link
Contributor Author

ypcode commented Jul 7, 2018

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jul 7, 2018

Thanks for helping out! For a first contribution, pretty well done 👏

@ypcode
Copy link
Contributor Author

ypcode commented Jul 16, 2018

Hi @waldekmastykarz ,

I adjusted as you requested, after messing up a bit because I forgot to rebase. it should be all fine now.

I will submit another PR for the add clientsidetext command with a refactored helper class

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jul 17, 2018

Cool! Thanks @ypcode! I will have a look shortly!

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jul 18, 2018

Hey @ypcode, could you please do me a favour? Could you please squash your commits into one? I'm getting a bunch of conflicts along the way when trying to do it after merging with the latest dev and I would hate to brake something you've built. Thanks in advance!

@ypcode
Copy link
Contributor Author

ypcode commented Jul 19, 2018

Hi @waldekmastykarz , does it go better now?
Sorry for some mess up with this, I squashed and it looks more clear now.
Tell me if I can do anything else :)

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jul 19, 2018

Ideally, your PR should say that you want to merge 1 commit. If you look at the top, it says at the moment that there are 6 commits. No worries and thanks for helping me process the changes. It will be cool to get them merged.

@ypcode
Copy link
Contributor Author

ypcode commented Jul 19, 2018

@ypcode
Copy link
Contributor Author

ypcode commented Jul 19, 2018

Hi @waldekmastykarz, that should be fine now, I see "wants to merge 1 commit" :), I forgot a + sign to override the remote branch, that's why we were still seeing the previous commits :)

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jul 19, 2018

Perfect! Thanks again @ypcode!

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jul 19, 2018

Merged manually with some additional fixes. Once again, thanks @ypcode and welcome aboard as an Office 365 CLI contributor 👏

@ypcode ypcode deleted the dev-addclientsidewebpart branch Jul 19, 2018
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

3 participants