-
Notifications
You must be signed in to change notification settings - Fork 3
docs: update the README with dev docs #46
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
docs: update the README with dev docs #46
Conversation
Thanks for the pull request, @tecoholic! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
@tecoholic Can you please fix the CI here, i think after that we are good to go. Thanks a lot for working on this. |
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.
@tecoholic I have added few comments
README.rst
Outdated
.. code-block:: sh | ||
|
||
pip install tutor-contrib-aspects | ||
tutor enable aspects |
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.
tutor enable aspects | |
tutor plugin enable aspects |
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.
and we should also add tutor config save --set ASPECTS_ENABLE_STUDIO_IN_CONTEXT_METRICS=True
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.
Do we really required to build openedx?
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.
@farhaanbukhsh The tutor-contrib-aspects
README has it under Rebuild Docker Images:
, which I think comes from this openedx-dev image patch that does pip install.
3. Clone this repo inside *frontend-app-authoring* and install it | ||
|
||
.. code-block:: | ||
|
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.
Maybe adding https://github.com/openedx/frontend-app-authoring?tab=readme-ov-file#startup here will make the setup simpler
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.
@farhaanbukhsh I thought about doing it. But I find it hard to switch repo READMEs and modify the just a couple of commands. I end up missing the order of the commands and having to redo it. So, I thought I will document everything here in order, so we can just follow a single readme.
@farhaanbukhsh The CI initially failed at It would be better to fix the CI and also update the dependencies to match the versions in authoring MFE, so we can drop the |
4015922
to
e1a2381
Compare
README.rst
Outdated
|
||
cd frontend-app-authoring | ||
git clone https://github.com/openedx/frontend-plugin-aspects.git | ||
npm install ./frontend-plugin-aspects --legacy-peer-deps |
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.
npm install ./frontend-plugin-aspects --legacy-peer-deps | |
npm install ./frontend-plugin-aspects |
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.
@farhaanbukhsh Updated 👍
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.
@tecoholic once you merge in my change, see if you can put the --legacy-peer-deps
back and check that it still works
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.
@saraburns1 Thanks a lot for the quick fix. We are intentionally trying to get rid of the --legacy-peer-deps
. So, if the CI passes now, we should be good.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
===========================================
+ Coverage 16.61% 48.50% +31.88%
===========================================
Files 15 15
Lines 331 334 +3
Branches 65 65
===========================================
+ Hits 55 162 +107
+ Misses 265 171 -94
+ Partials 11 1 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
@tecoholic just one more change and we should be good to merge this.
1f5a5f8
to
ea4c615
Compare
@farhaanbukhsh I have applied the change you suggested. However, the CI is failing due to renovate-bot's auto updated of react-query. |
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.
👍
- ✅ I tested this by reading through it and following the instructions
- ✅ I read through the code
- ❌ I checked for accessibility issues
- ✅ Includes documentation
@tecoholic thank you so much for the amazing job at the documentation. |
Updates the placeholders in the README.