edx-cookiecutters icon indicating copy to clipboard operation
edx-cookiecutters copied to clipboard

feat: prompt messages

Open CodeWithEmad opened this issue 11 months ago • 17 comments

Merge checklist:

Check off if complete or not applicable:

  • [x] Changelog record added
  • [x] Documentation updated (not only docstrings)
  • [x] Fixup commits are squashed away
  • [x] Unit tests added/updated
  • [x] Manual testing instructions provided
  • [x] Noted any: Concerns, dependencies, deadlines, tickets

This pull request adds "Prompt" messages to all cookie cutters, improving the template creation experience. Additionally, some cleanup was performed.

CodeWithEmad avatar Feb 26 '24 12:02 CodeWithEmad

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

openedx-webhooks avatar Feb 26 '24 12:02 openedx-webhooks

Hey @CodeWithEmad, thank you for this contribution! I'll line them up for a test run now.

itsjeyd avatar Mar 08 '24 14:03 itsjeyd

Hey @CodeWithEmad, looks like you got some failing tests here. Let us know when you've had a chance to address those.

itsjeyd avatar Apr 12 '24 13:04 itsjeyd

Sure @itsjeyd. My changes were pretty harmless :) I'll look into it.

CodeWithEmad avatar Apr 13 '24 11:04 CodeWithEmad

@itsjeyd Tests are failing due to missing packages, possibly caused by Dependabot (also see https://github.com/openedx/edx-cookiecutters/pull/448). Fixed in https://github.com/openedx/edx-cookiecutters/pull/451. Probably mine will be fixed too after merging 451.

CodeWithEmad avatar Apr 16 '24 12:04 CodeWithEmad

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 master by now (?). If so, the next step here would probably be to rebase and see if that gets the build to pass :)

itsjeyd avatar Apr 22 '24 07:04 itsjeyd

Hey @CodeWithEmad, just checking in to see if you're still planning to continue working on this PR?

itsjeyd avatar May 23 '24 15:05 itsjeyd

Sorry for the late response, @itsjeyd. I forgot about it. will be fixed shortly.

CodeWithEmad avatar May 25 '24 06:05 CodeWithEmad

@CodeWithEmad No problem! Looks like we've got a green build here now. Marking as ready for review 🚀

itsjeyd avatar May 31 '24 07:05 itsjeyd

Hey @robrap, would you or someone else from @openedx/2u-arch-bom have capacity to review this PR?

itsjeyd avatar May 31 '24 07:05 itsjeyd

Reached out to core contributors via Slack.

itsjeyd avatar Jun 26 '24 12:06 itsjeyd

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.

robrap avatar Jun 26 '24 12:06 robrap

Thanks @robrap and @feanil.

itsjeyd avatar Jun 27 '24 08:06 itsjeyd

Hi @feanil. Thanks for the review.

What's the argument for making a switch to code blocks for all of this?

Using .. code-block:: LANGUAGE provides language-specific syntax highlighting, which enhances readability, especially on GitHub. While this might not be as significant our in Sphinx docs, it ensures clarity and consistency across all platforms.

Also having the prefix $ is a good indicator that the commands are meant to be run

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 .rst files on GitHub provides some syntax highlighting, but employing a code-block will automatically add a copy button. When this button is used, having the $ at the beginning of each command forces users to manually remove that character, which can detract from their user experience. Most users will already understand that these are terminal commands, so removing the $ can streamline the process and improve usability.

image

CodeWithEmad avatar Jun 28 '24 11:06 CodeWithEmad

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.

robrap avatar Jun 28 '24 15:06 robrap

Awesome. I'll make sure Ill read it and depending on the result, I'll try to come up with changes for the OEP.

CodeWithEmad avatar Jun 28 '24 16:06 CodeWithEmad

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. :)

robrap avatar Jun 28 '24 16:06 robrap

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

openedx-webhooks avatar Jul 15 '24 19:07 openedx-webhooks