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 new script sample 'lists-teams-decluttering' #5575

Closed
wants to merge 9 commits into from
Closed

Adds new script sample 'lists-teams-decluttering' #5575

wants to merge 9 commits into from

Conversation

tmaestrini
Copy link

No description provided.

@Adam-it
Copy link
Member

Adam-it commented Oct 17, 2023

@tmaestrini thanks for opening the PR. We will review it ASAP.
Is this PR connected to some issue?

@tmaestrini
Copy link
Author

tmaestrini commented Oct 18, 2023

Thanks, @Adam-it. No, it's only a new basic sample that I wrote. It is based on practical needs.

@milanholemans milanholemans marked this pull request as draft October 18, 2023 11:48
@tmaestrini tmaestrini marked this pull request as ready for review October 18, 2023 14:22
@tmaestrini
Copy link
Author

@milanholemans does my changes fit? Or do you want me to fix anything else?

@milanholemans
Copy link
Contributor

Looks good at first sight @tmaestrini 👍

@tmaestrini
Copy link
Author

Do you want me to do sth else, @Adam-it ?

@Adam-it
Copy link
Member

Adam-it commented Oct 24, 2023

Do you want me to do sth else, @Adam-it ?

Hi, rather not 😉.
Sorry for the hold up. Please be a bit more patient before we process your PR 🙏👍

@Adam-it Adam-it self-assigned this Oct 28, 2023
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@tmaestrini awesome work 👏.
I love the sample 😍
Lets work on a couple of details around it before we merge 👍

@Adam-it Adam-it marked this pull request as draft October 28, 2023 21:24
@Adam-it Adam-it added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 28, 2023
@tmaestrini
Copy link
Author

tmaestrini commented Oct 29, 2023

@Adam-it Thanks for the feedback! 🙏
I've fixed all the details according to your orders – please see current version…
The sample script in https://pnp.github.io/cli-microsoft365/contribute/script-sample/new-script-sample should be updated to fit the specification.

@Adam-it
Copy link
Member

Adam-it commented Oct 29, 2023

@Adam-it Thanks for the feedback! 🙏
I've fixed all the details according to your orders – please see current version…
The sample script in https://pnp.github.io/cli-microsoft365/contribute/script-sample/new-script-sample should be updated to fit the specification.

Thanks for the quick turn around. I will give it a check ASAP.

What do you see lacking in the script sample? Maybe we could add this to this PR along the way 🙂

@tmaestrini
Copy link
Author

Dear @Adam-it, thank you – really appreciated!

The lacking in the template script is a minor thing; it concerns the indentation starting from line 104 to 106 and 110 to 112.

@Adam-it
Copy link
Member

Adam-it commented Nov 1, 2023

Dear @Adam-it, thank you – really appreciated!

The lacking in the template script is a minor thing; it concerns the indentation starting from line 104 to 106 and 110 to 112.

I see.
I say if you still have time we could add this small change to this PR as well 👍. If not I will do it when merging

@Adam-it Adam-it marked this pull request as ready for review November 1, 2023 22:17
@Adam-it
Copy link
Member

Adam-it commented Nov 1, 2023

@tmaestrini if I understood you correctly you are ready for another round right? I marked this PR as 'ready to review' and planning to do so in the upcoming weekend 👍

@tmaestrini
Copy link
Author

tmaestrini commented Nov 2, 2023

Dear @Adam-it, thank you – really appreciated!
The lacking in the template script is a minor thing; it concerns the indentation starting from line 104 to 106 and 110 to 112.

I see. I say if you still have time we could add this small change to this PR as well 👍. If not I will do it when merging

Hey @Adam-it! No need anymore, @milanholemans fixed it in his commit: d781816

@Adam-it
Copy link
Member

Adam-it commented Nov 3, 2023

Hey @Adam-it! No need anymore, @milanholemans fixed it in his commit: d781816

Looks like someone was monitoring our conversation 😅. Cool, we have it sorted out. I will try to process this PR during the weekend 👍

@milanholemans
Copy link
Contributor

milanholemans commented Nov 3, 2023

I was aware of this issue since my last PR with the update of the syntax was merged. However, I wasn't able to update it until a few days ago ☺️

Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

Perfect 👏

@Adam-it
Copy link
Member

Adam-it commented Nov 5, 2023

Ready to merge 🚀

@tmaestrini
Copy link
Author

Thank you, @Adam-it! 🙌 the merge will be done by your team, right? So my work is done "for now"?

@Adam-it
Copy link
Member

Adam-it commented Nov 5, 2023

Thank you, @Adam-it! 🙌 the merge will be done by your team, right? So my work is done "for now"?

yep. I am doing it now together with other PRs. Should be live in 10-20 min

@Adam-it
Copy link
Member

Adam-it commented Nov 5, 2023

merged manually. Thank you for your awesome work👏
You Rock 🤩

@Adam-it Adam-it closed this Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later pr-merged pr-sample
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants