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

Change readme gif and contributing guidelines. Closes #5564 #5570

Closed
wants to merge 1 commit into from

Conversation

Aaron-Junker
Copy link
Contributor

@Aaron-Junker Aaron-Junker commented Oct 16, 2023

I hope the quality of the new gif is ok enough.

@Adam-it
Copy link
Member

Adam-it commented Oct 16, 2023

Thank @Aaron-Junker for opening the PR. We will review it ASAP.

@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.

I think it is a good start but we should recheck the details.
I am not sure if clearing out the guidance is the way to go here. @milanholemans what is your take on that?
Also the new gif is ok but it just seems oldfashioned. @pnp/cli-for-microsoft-365-maintainers anyone any other feedback on that?

- **DO NOT** surprise us with big PR's. Instead file an issue & start a discussion so we can agree on a direction before you invest a large amount of time.
- **DO NOT** commit code you didn't write.
- **DO NOT** submit PR's that refactor existing code without a discussion first.
You can find these guidelines here: <https://pnp.github.io/cli-microsoft365/contribute/contributing-guide>
Copy link
Member

Choose a reason for hiding this comment

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

Since it's .md we may hide the link behind a label. I guess there is no need to present the link right?

Suggested change
You can find these guidelines here: <https://pnp.github.io/cli-microsoft365/contribute/contributing-guide>
You can find these [guidelines here](https://pnp.github.io/cli-microsoft365/contribute/contributing-guide)

@@ -2,53 +2,4 @@

We appreciate that you're interested in helping with moving the project forward. Before you submit your first PR, please read the following guide. We'd hate to see you work on something that someone else is already working on, something that we agreed not to do or something that doesn't match the project.

Sharing is caring!
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe the aim of this issue was to remove most of the guidance here but rather update it to align with the current approach and add a link to full details.
@milanholemans as you are the author of the issue then maybe you may confirm.
in many places the Contribuiting.md is tracked and shown as a pointer to contribution guidance. If we just clear out this file and only leave the link it will only become problematic for someone who is in search of this guidance as now he or she would need to actually first redirect to contribuiting.md and then redirect again to docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't read the entire contributing page, but I guess most of the things there are still relevant and we should only update links to the GH wiki.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I think we could do better than that. The blue terminal seems kinda oldschool and reminds me of Windows PowerShell (the newer PowerShell 7 is also black terminal).
I think it would be best if we could show that CLI works in multiple shells. In the past we had something similar done by @garrytrinder, maybe you could advise.
@Aaron-Junker if you want we may leave the current gif for now and in this PR only focus on updating the guidance in order not to keep it on hold for much longer

@Adam-it Adam-it marked this pull request as draft November 5, 2023 21:58
@Adam-it
Copy link
Member

Adam-it commented Nov 18, 2023

@Aaron-Junker are you working on this PR? Do you need any help?

@Aaron-Junker
Copy link
Contributor Author

@Aaron-Junker are you working on this PR? Do you need any help?

@Adam-it Still working on it. Just had little time last month.

@Adam-it
Copy link
Member

Adam-it commented Dec 5, 2023

@Aaron-Junker are you working on this PR? Do you need any help?

@Adam-it Still working on it. Just had little time last month.

sure thing 👍. Thanks for the response and keeping us updated. You rock 🤩

@Adam-it
Copy link
Member

Adam-it commented Jan 7, 2024

@Aaron-Junker Happy New Year 🤩🥳
I hope everything is fine on your end 🙂
Do you think we could try to recheck this issue/PR?
It's totally fine if you are not in a position to finish it off now. It's totally understandable and we may just close this PR. The branch will be still present on your local fork so it should be quite easy to restart if the issue is still free and you would like to continue.

@Adam-it
Copy link
Member

Adam-it commented Jan 22, 2024

Closing due to lack of response 😢
If anything changes we may always reopen 👍

@Adam-it Adam-it closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants