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 'page text add' command + bug fix in modern page section handling + review #556

Closed
wants to merge 3 commits into from
Closed

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented Aug 21, 2018

  • Adds the new spo page text add command
  • Fixes bugs in Modern Pages manipulation (handling of sections and columns)
  • Adds method to handle insertion of a control (text or WebPart) at a specific position in a column
  • Reviewed specs of code in which clientsidepage.ts file code is used

@coveralls
Copy link

coveralls commented Aug 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 1ec42b9 on ypcode:dev-text-add into a1aa66e on pnp:dev.

@ypcode
Copy link
Contributor Author

ypcode commented Aug 21, 2018

Sorry... needed to fix a copy/paste pattern issue I missed... :D

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 22, 2018

No problem. Thanks for the PR and I'll have a look shortly!

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 26, 2018

Hey @ypcode, why the ClientSidePageCommandHelper rather than issuing the required requests directly in the command? As for the helper methods, aren't they already available through https://github.com/pnp/office365-cli/blob/dev/src/o365/spo/commands/page/Page.ts and https://github.com/pnp/office365-cli/blob/dev/src/o365/spo/commands/page/clientsidepages.ts?

@ypcode
Copy link
Contributor Author

ypcode commented Sep 2, 2018

Hi @waldekmastykarz , actually I did not realize there was this new Page.ts helper class, the helper class I added was implemented several weeks ago but I submitted the PR once everything ready. Too bad with ended up with some "duplicated" code there...

I wrote this class to handle the exact same behavior in both WebPart and text add commands, reviewing it lastly I realized some code is specific to each commands...

For the clientsidepage.ts, the code borrowed from pnpjs, I thought it was safer not to touch it too much and use process the arguments (such as column and sections in a dedicated location, and since I need the exact same thing for at least two commands, I put it into a helper class...

Let me refactor the real common code into Page.ts and the specific code in the appropriate commands :)

[EDIT] I don't really like to pass a big amount of arguments as in the getPage method of the Page.ts, I'd rather use a dedicated 'context' object, but it's up to you @waldekmastykarz , what do you prefer ?

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Sep 3, 2018

Thanks for the background info. Ideally, we should keep the clientsidepage.ts file as close to the original as possible, so that we can easily update it if it's updated in PnPjs.
Regarding Page.ts, it makes perfect sense to consolidate your changes with it.
As for the getPage method, I'd prefer to keep the different commands consistent and not introduce command-specific contexts which would complicate maintaining the code and decrease reusability of code in the CLI.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Nov 4, 2018

Hey @ypcode, are you still working on this?

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Nov 15, 2018

Closing it for now due to lack of response.

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

Successfully merging this pull request may close these issues.

None yet

3 participants