New command Add Client Side WebPart to Modern Page #502
Conversation
|
Thanks @ypcode, will have a look shortly |
There was a problem hiding this comment.
Hey @ypcode, would you mind doing a few adjustments? Thanks!
| ClientSidePageComponent, | ||
| CanvasColumn | ||
| } from './clientsidepages'; | ||
| import { StandardWebPart, StandartWebPartUtils } from './StandardWebpartTypes'; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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>', |
There was a problem hiding this comment.
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>', |
There was a problem hiding this comment.
webPartId is optional so let's use [webPartId] instead of <webPartId>
| } | ||
| } | ||
|
|
||
| if (args.options.section && args.options.section < 1) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
we should add validation to ensure that the specified value is a number
|
Forgot to mention: the |
|
Hi Waldek,
Sure, I’ll do it in a few days :) thanks for the great review :)
…________________________________
From: Waldek Mastykarz <notifications@github.com>
Sent: Friday, July 6, 2018 7:53:46 PM
To: pnp/office365-cli
Cc: Yannick Plenevaux; Mention
Subject: Re: [pnp/office365-cli] New command Add Client Side WebPart to Modern Page (#502)
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#502 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AH9X1FouyxSREHiPtut__G-RElKGHjlbks5uD6QqgaJpZM4U2RCY>.
|
|
Thanks for helping out! For a first contribution, pretty well done 👏 |
|
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 |
|
Cool! Thanks @ypcode! I will have a look shortly! |
|
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! |
|
Hi @waldekmastykarz , does it go better now? |
|
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. |
|
I initially planned to have a single commit :p, here did I squash the big amount of commits I did wrong when committing adjustments. I can work it further if you wish but not before this evening.
Sorry for the issues...
At least I am gaining experience with git intricacies :p
|
|
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 :) |
|
Perfect! Thanks again @ypcode! |
|
Merged manually with some additional fixes. Once again, thanks @ypcode and welcome aboard as an Office 365 CLI contributor 👏 |
[EDIT] I forgot to mention the related issue #366