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

Updating the README.md #38

Merged
merged 11 commits into from
Sep 8, 2020
Merged

Updating the README.md #38

merged 11 commits into from
Sep 8, 2020

Conversation

yunjchoi
Copy link
Contributor

@yunjchoi yunjchoi commented Sep 6, 2020

Updating the README.md to include information on prerequisites, installation, and feature set.

Updating the README.md to include information on prerequisites, installation, and feature set.
@yunjchoi
Copy link
Contributor Author

yunjchoi commented Sep 6, 2020

Hi @Tatsinnit and @itowlson,
Can you review my PR? Thanks!

@Tatsinnit
Copy link
Member

Tatsinnit commented Sep 7, 2020

💡Cool, Thank you for the ping. Couple of observations + minor note:

📓 I was thinking that there will be an MSDN blog entry which we can link with the feature mention in the ReadMe, but I am super easy with any approach to cater right audience. cc: @itowlson might have some idea with regards to the current ReadMe changes. If, we are going towards more depth of how the whole login et. al. works, what do you guys think of utilising some of the screenshots with the details: like here: #6

Observation:

  • Instead of master to master merge I would suggest do this:
    • wherever your local branch is from master you could do: checkout -b feature/some_meaningful_ name_of_this_work
    • git push
    • Please let me know and feel free to teams call if that will help.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

This looks good - so nice to have someone putting in the documentation effort to give it that "finished product" look! Thank you!

I've suggested a few changes but they're pretty nitpicky - feel free to skip over them if time is short or if you disagree.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Switching back to the bullet form + removing prerequisites section
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Looks good!

@itowlson itowlson added the do-not-merge Not ready for merge yet (but ready for testing and review) label Sep 7, 2020
@itowlson
Copy link
Contributor

itowlson commented Sep 7, 2020

Adding do-not-merge label to track that there are a couple of placeholders that are waiting for screenshots.

@Tatsinnit
Copy link
Member

Tatsinnit commented Sep 8, 2020

Thanks, cc: @itowlson + @yunjchoi - looks good to me now, please eye ball if there is anything else left otherwise from my end this looks ready-to-merge 🙏

  • 💡 Please note: this work kind of covers need for this issue here: 'Log in' documentation #7 - So we can link this issue and close it as part of this read-me change. What do you guys think?

@itowlson itowlson removed the do-not-merge Not ready for merge yet (but ready for testing and review) label Sep 8, 2020
@itowlson itowlson merged commit d6c76f7 into Azure:master Sep 8, 2020
@itowlson
Copy link
Contributor

itowlson commented Sep 8, 2020

🎉

@itowlson itowlson mentioned this pull request Sep 8, 2020
tejhan pushed a commit to tejhan/vscode-aks-tools that referenced this pull request Dec 4, 2024
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.

3 participants