-
Notifications
You must be signed in to change notification settings - Fork 317
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 text to modern page #365
Comments
@waldekmastykarz I can take this one if you're happy with #366 |
Please do! 👏 |
Ok, I will pretty soon. Quick question, since most of the code would be copy of the clientsidewebpart add command, do you prefer to refactor, or to copy/paste the code ? if refactoring, should I go for an base class or a helper class to consume ? |
Let's go with a helper. That gives us more flexibility in case we need to refactor in the future |
Hi @waldekmastykarz , just to keep you up to date, I am a bit delayed on that, but I started refactoring and had to refactor the specs as well. But still on going, don't worry :) By the way, what do you think of closing the #366 PR and process the upcoming one with the WebPart and the Text commands ? |
Thanks for the update @ypcode. I'm processing open PRs typically once a week. I try not to add new commands just before a release to avoid last-minute issues, which is why I haven't processed your PR yet. Still, it's on my to-do list for the coming weekend, and if all is well, will be included in the next beta. |
Don't worry, that is totally fine, no rush here :) |
You can include the same file in the next PR and when I'll merge them, it will become one and the same. |
Hi, Just a quick update on this, I implemented the new command, but I found a quite significant bug when playing with sections (the layout of the page is being screwed up) at this is probably also present in the clientsidewebpart command. However, some tests are now failing in other commands, I have to figure out all these before submitting the PR :) Regards, |
Thanks for the update @ypcode! Have you checked if the bug is fixed in PnPjs from which we borrowed the code? If so, it would be great if you let the guys know and share your findings. |
Yes, it is fixed on their side since I actually ported back the latest implementation of the clientsidepage file with some changes to remove the pnpjs dependencies, that is what actually causes some tests to fail... BTW, I also added one method to properly handle the --order argument. I realized the specs say it is the order of the section, but actually it should be "the order of the control", I will make the changes accordingly |
Cool! Glad that we have tests that verify how things work |
Hey @waldekmastykarz, actually, As I recopied manually all my changes to a new branch from the updated dev trunk, I forgot to recopy the updated PnPJS class that has fix many issues. Will make this work ASAP |
Perhaps we're looking at different scenarios. If the issue is with PnPjs, rather than just fixing it in our copy, it would be better, to log it in the PnPjs repo. Patrick and the crew spent significantly more time dealing with modern page HTML than us so they can be a better judge of whether it's really an issue and how it should be fixed, or we're just using it in an odd way. |
Will definitely talk with them once I resynced with the latest version of the clientsidepage class. What a pity I actually lost the changes I already started to bring... |
Hi @waldekmastykarz , I resynced the clientsidepage class from pnpjs. Everything works fine except that many tests are now failing mainly because the expected HTML was erroneous (there was a single closing div tag in the html returned by the pnpJS code. The branch is available on my own repo : https://github.com/ypcode/office365-cli/tree/dev-spo-page-text-add It takes a lot of time to correct the failing tests even if the commands still work, and unfortunately I will lack some... As I understood you were waiting on it, do you want me to submit it as PR with the tests failing ? I will also need later to refactor the thing with WebPart to properly handle inserting them with a specific order |
I can grab the code from your fork and work on that. You will of course get the credit for your work 😊 With regards to properly handling web part order, I'd expect to be handled correctly by PnPjs. If it's not, then we should raise an issue there and sync the updated version which will help a broader audience benefit of the fix. Once again thanks for your help and I appreciate your time 👏 |
Sounds good 😃 I will also make sure, if not handled in the meantime, the WebParts command works fine with order + need some refactoring also... Thanks for this wonderful and very addictive project, you're leading and Thank YOU very much for your acknowledgement! It's always good to receive from a star like you 😄 |
It's not always easy to prioritize hobby side-project with everything else in life, so when we do get a contribution, we appreciate it, no matter how small it is. |
FWIW: I had a look at the order option and it turns out that it never worked. Like you mentioned, it isn't there and the order option specified while adding web part is ignored. We'll be able to make a good use of your code so far! 👏 |
Hi waldek, will definitely need to refactor the webpart command as well then ;) if it can wait a few days I’d like to do it :)
…________________________________
De : Waldek Mastykarz <notifications@github.com>
Envoyé : samedi, décembre 15, 2018 9:34 PM
À : pnp/office365-cli
Cc : Yannick Plenevaux; Assign
Objet : Re: [pnp/office365-cli] New command: add text to modern page (#365)
FWIW: I had a look at the order option and it turns out that it never worked. Like you mentioned, it isn't there and the order option specified while adding web part is ignored. We'll be able to make a good use of your code so far! 👏
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#365 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AH9X1Oaz2m8MVaL_Hh6Po7od_8Ciz24Qks5u5VzigaJpZM4SpPrQ>.
|
And I will also PR it to pnpjs ;)
…________________________________
De : yannick Plenevaux <yannick.plenevaux@gmail.com>
Envoyé : dimanche, décembre 16, 2018 3:33 PM
À : pnp/office365-cli; pnp/office365-cli
Cc : Assign
Objet : Re: [pnp/office365-cli] New command: add text to modern page (#365)
Hi waldek, will definitely need to refactor the webpart command as well then ;) if it can wait a few days I’d like to do it :)
________________________________
De : Waldek Mastykarz <notifications@github.com>
Envoyé : samedi, décembre 15, 2018 9:34 PM
À : pnp/office365-cli
Cc : Yannick Plenevaux; Assign
Objet : Re: [pnp/office365-cli] New command: add text to modern page (#365)
FWIW: I had a look at the order option and it turns out that it never worked. Like you mentioned, it isn't there and the order option specified while adding web part is ignored. We'll be able to make a good use of your code so far! 👏
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub<#365 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AH9X1Oaz2m8MVaL_Hh6Po7od_8Ciz24Qks5u5VzigaJpZM4SpPrQ>.
|
There is no rush for it as so far no one spotted it. I can log an issue for it and assign it to you. Thanks again for bringing it up. |
Add text to modern page
spo page text add -n|--pageName <pageName> -u|--webUrl <webUrl> -t|--text <text> --section [section] --column [column] --order [order]
For more information see the Add-PnPClientSideText cmdlet.
The text was updated successfully, but these errors were encountered: