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

Adds 'spo page remove' command #557

Closed
wants to merge 2 commits into from
Closed

Adds 'spo page remove' command #557

wants to merge 2 commits into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented Aug 22, 2018

Issue #363

  • Adds the spo page remove command
  • Specs for the command

@coveralls
Copy link

coveralls commented Aug 22, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 9f3e051 on ypcode:dev-page-remove into 7f50b9a on pnp:dev.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 22, 2018

Thanks! I'll have a look shortly!

@waldekmastykarz waldekmastykarz self-assigned this Aug 22, 2018
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Could you add the missing docs page to the PR? The other fixes I mentioned are pretty small so I could fix them myself when merging the PR or you can fix them yourself if you like

@@ -91,6 +91,7 @@ nav:
- navigation node remove: 'cmd/spo/navigation/navigation-node-remove.md'
- page:
- page add: 'cmd/spo/page/page-add.md'
- page remove: 'cmd/spo/page/page-remove-md'
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 26, 2018

Choose a reason for hiding this comment

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

I'm afraid you didn't include this page in the PR

Copy link
Contributor Author

@ypcode ypcode Sep 2, 2018

Choose a reason for hiding this comment

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

Done

@@ -91,6 +91,7 @@ nav:
- navigation node remove: 'cmd/spo/navigation/navigation-node-remove.md'
- page:
- page add: 'cmd/spo/page/page-add.md'
- page remove: 'cmd/spo/page/page-remove-md'
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 26, 2018

Choose a reason for hiding this comment

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

Entries are sorted alphabetically, so ideally page remove should go between page list and page set.

Copy link
Contributor Author

@ypcode ypcode Sep 2, 2018

Choose a reason for hiding this comment

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

Done

const options: CommandOption[] = [
{
option: '-n, --name <name>',
description: 'Name of the page to create'
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 26, 2018

Choose a reason for hiding this comment

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

Should be Name of the page to remove

Copy link
Contributor Author

@ypcode ypcode Sep 2, 2018

Choose a reason for hiding this comment

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

Done

},
{
option: '-u, --webUrl <webUrl>',
description: 'URL of the site where the page should be created'
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 26, 2018

Choose a reason for hiding this comment

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

Should be URL of the site where the page should be removed

Copy link
Contributor Author

@ypcode ypcode Sep 2, 2018

Choose a reason for hiding this comment

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

Done

@ypcode
Copy link
Contributor Author

ypcode commented Aug 26, 2018

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 26, 2018

No problem 👍

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Sep 3, 2018

Thanks!

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Sep 6, 2018

Merged manually. Thank you!

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