-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: prompt messages #441
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. |
Hey @CodeWithEmad, thank you for this contribution! I'll line them up for a test run now. |
Hey @CodeWithEmad, looks like you got some failing tests here. Let us know when you've had a chance to address those. |
Sure @itsjeyd. My changes were pretty harmless :) I'll look into it. |
Thanks @CodeWithEmad. It looks like #451 was closed as obsolete a few hours ago. Which might mean that some other PR containing relevant changes has landed on |
Hey @CodeWithEmad, just checking in to see if you're still planning to continue working on this PR? |
Sorry for the late response, @itsjeyd. I forgot about it. will be fixed shortly. |
@CodeWithEmad No problem! Looks like we've got a green build here now. Marking as ready for review 🚀 |
Hey @robrap, would you or someone else from @openedx/2u-arch-bom have capacity to review this PR? |
Reached out to core contributors via Slack. |
Apologies that I missed your ping. My team no longer maintains this repo. The repo should designate the maintainer if there is one. I’ll ask. |
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.
The double colon is a valid way to have a piece of block text following some standrad text. What's the argument for making a switch to code blocks for all of this? Also having the prefix $
is a good indicator that the commands are meant to be run in a terminal by hand, so I'm not sure it makes sense to remove them.
Hi @feanil. Thanks for the review.
Using
While the $ prefix can indicate that commands are intended to be run in a terminal, it's important to consider the overall user experience. Using a double colon in |
Thanks @CodeWithEmad. Those seem like reasonable arguments to me, but we'll see how @feanil feels. Note that code blocks that include both a command and example output would be a different case, where having the prompt helps differentiate. Depending on where this lands, we might want to consider adding thoughts on https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0019-bp-developer-documentation.html. |
Awesome. I'll make sure Ill read it and depending on the result, I'll try to come up with changes for the OEP. |
I don't think this is addressed much in the OEP at this point, but it could be an enhancement, depending on where we land. Also, you could use the OEP process to propose these best practices (or start a thread on the forums), if you want more than 3 opinions. :) |
@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. |
Merge checklist:
Check off if complete or not applicable:
This pull request adds "Prompt" messages to all cookie cutters, improving the template creation experience. Additionally, some cleanup was performed.