-
Notifications
You must be signed in to change notification settings - Fork 59
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
chore: fixed typos #482
chore: fixed typos #482
Conversation
Thanks for the pull request, @CodeWithEmad! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@CodeWithEmad this has some conflicts with your other PR that I merged, can you fix it up? |
Sure @feanil. |
01287a2
to
28e335c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this @CodeWithEmad! It really improves the quality of the docs, and I appreciate the fixes to my own typos 😳.
I left a couple of comments to fix up the grammar in a few places and a couple of nits that you can take or leave.
I have one style question that I'd like a second opinion on, maybe from @feanil. The existing docs often include a shell prompt ($
) in code snippets, and this PR removes them. I think it's an improvement, but we should check on what is the common practice.
source/developers/references/developer_guide/extending_platform/xblocks.rst
Outdated
Show resolved
Hide resolved
source/developers/references/developer_guide/internationalization/i18n.rst
Show resolved
Hide resolved
Thank you @pdpinch for the review. I've added all the requested changes.
The code blocks have a copy button in the website. Having the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I'm comfortable merging this, but I had one last question for @feanil -- do the 2 commits need to be squashed, or can I do a rebase and merge them? (There doesn't seem to be any commit linting in this repo, but I think they each pass conventional commits)
You can rebase merge, I only squash when it's clear there are a lot of intermediate temp commits. We turned off conventional commits on this repo to make it easier for less technical folks to make suggestions without needing to understand conventional commits. |
@CodeWithEmad 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Thank you @CodeWithEmad! |
As decided in #474, I'll break the big clean-up PR into smaller PRs for faster review/merge.
this will introduce: