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.

Sign up for GitHub

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

Jump to bottom

docs: update for OEP-58 implementation - FC-55 and FC-12 #540

Merged
merged 2 commits into from
Sep 23, 2024
Merged

docs: update for OEP-58 implementation - FC-55 and FC-12 #540

merged 2 commits into from
Sep 23, 2024

Conversation

OmarIthawi
Copy link
Member

@OmarIthawi OmarIthawi commented Jul 20, 2024 •
edited
Loading

Docs update after OEP-58 has been implemented.

See:

@openedx-webhooks
Copy link

openedx-webhooks commented Jul 20, 2024 •
edited
Loading

Thanks for the pull request, @OmarIthawi!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/docs-openedx-org-maintainers. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 20, 2024
@OmarIthawi OmarIthawi force-pushed the oep-58-closure-updates branch from b185f5c to 3947c42 Compare July 20, 2024 15:06
sarina
sarina approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@sarina sarina left a comment

Choose a reason for hiding this comment

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

I haven't tested these instructions, but they're clear and easy to read

@sarina
Copy link
Contributor

sarina commented Aug 13, 2024

@brian-smith-tcril would you take a quick read through and merge if you're satisfied?

@OmarIthawi
Copy link
Member Author

@brian-smith-tcril @sarina would you mind triaging this PR?

sarina
sarina reviewed Aug 28, 2024
View reviewed changes
source/developers/how-tos/enable-translations-new-repo.rst Outdated
.. _transifex.yml: https://github.com/openedx/openedx-translations/blob/main/transifex.yml
.. _Drag and Drop XBlock transifex.yml entry: https://github.com/openedx/openedx-translations/blob/19c0fcbbc334c56022df355fa5b529e5853d30f9/transifex.yml#L253-L259
.. _edx-platform e7fc0c6: https://github.com/openedx/edx-platform/commit/e7fc0c6b6f8b44fa4bcc71d00ae2931f91cc664c
.. _edx-platform Redwood: https://github.com/openedx/edx-platform/commit/e7fc0c6b6f8b44fa4bcc71d00ae2931f91cc664c
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a tag?

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the naming is just confusing here. The link is to the commit feat: 'atlas pull' for XBlock translations | FC-0012. Linking to the tagged version wouldn't provide the same context as linking to this commit.

I think another possible option would be to link to the PR (openedx/edx-platform#33698), so up on line 48 where it says "Redwood or newer" we could change that to be "A version of edx-platform that includes PR 33698 (Redwood or newer)."

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, now Redwood is behind us. Removing the link altogether makes more sense.

source/developers/how-tos/enable-translations-new-repo.rst Outdated Show resolved Hide resolved
source/developers/how-tos/enable-translations-new-repo.rst Outdated Show resolved Hide resolved
@OmarIthawi OmarIthawi force-pushed the oep-58-closure-updates branch from fae14cb to 97945eb Compare September 19, 2024 18:18
OmarIthawi
OmarIthawi commented Sep 19, 2024
View reviewed changes
Copy link
Member Author

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @brian-smith-tcril @sarina for the review and sorry for the delay. Now it's ready with all notes addressed.

source/developers/how-tos/enable-translations-new-repo.rst Outdated
.. _transifex.yml: https://github.com/openedx/openedx-translations/blob/main/transifex.yml
.. _Drag and Drop XBlock transifex.yml entry: https://github.com/openedx/openedx-translations/blob/19c0fcbbc334c56022df355fa5b529e5853d30f9/transifex.yml#L253-L259
.. _edx-platform e7fc0c6: https://github.com/openedx/edx-platform/commit/e7fc0c6b6f8b44fa4bcc71d00ae2931f91cc664c
.. _edx-platform Redwood: https://github.com/openedx/edx-platform/commit/e7fc0c6b6f8b44fa4bcc71d00ae2931f91cc664c
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, now Redwood is behind us. Removing the link altogether makes more sense.

brian-smith-tcril
brian-smith-tcril approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

LGTM!

sarina
sarina approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@sarina sarina left a comment

Choose a reason for hiding this comment

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

I'll merge once the devstack ref is removed

source/developers/how-tos/enable-translations-new-repo.rst Outdated
@@ -274,5 +273,4 @@ After adding a repository to the `openedx-translations repo`_ verify the followi

.. _Tutor: https://docs.tutor.overhang.io/
.. _devstack: https://github.com/openedx/devstack/
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Is this reference now unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, now done!

@OmarIthawi OmarIthawi force-pushed the oep-58-closure-updates branch from 97945eb to a1198f3 Compare September 23, 2024 15:28
@sarina sarina merged commit 8462f9b into openedx:main Sep 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brian-smith-tcril brian-smith-tcril brian-smith-tcril approved these changes

@sarina sarina sarina approved these changes

Assignees
No one assigned
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@OmarIthawi @openedx-webhooks @sarina @brian-smith-tcril