-
Notifications
You must be signed in to change notification settings - Fork 80
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] Readme updated according OEP-55 #526
Conversation
Thanks for the pull request, @johnvente! 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. |
@arbrandes - hi there! are you able to re-run the tests for this? |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #526 +/- ##
==========================================
+ Coverage 74.70% 81.32% +6.61%
==========================================
Files 109 234 +125
Lines 2107 3812 +1705
Branches 519 873 +354
==========================================
+ Hits 1574 3100 +1526
- Misses 505 675 +170
- Partials 28 37 +9 ☔ View full report in Codecov by Sentry. |
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 for the PR! I made a few requests for changes.
README.rst
Outdated
In the following steps, replace "[PLACEHOLDER]" with the name of the repo you | ||
created when copying this template above. |
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.
This is only relevant to frontend-template-application, not individual MFEs.
README.rst
Outdated
2. Use node v18.x. | ||
|
||
The current version of the micro-frontend build scripts support node 18. | ||
Using other major versions of node *may* work, but this is unsupported. For | ||
convenience, this repository includes an .nvmrc file to help in setting the | ||
correct node version via `nvm <https://github.com/nvm-sh/nvm>`_. |
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 think replacing this with instructions to run nvm use
would be enough. (Provided this repo has an .nvmrc
, which seems to be the case.)
README.rst
Outdated
4. Update the application port to use for local development: | ||
|
||
Default port is 2001. If this does not work for you, update the line | ||
`PORT=2001` to your port in all .env.* files | ||
|
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.
Only relevant to frontend-template-app.
README.rst
Outdated
For anything non-trivial, the best path is to open an issue in this repository | ||
with as many details about the issue you are facing as you can provide. | ||
|
||
https://github.com/openedx/frontend-app-learning/issues |
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.
https://github.com/openedx/frontend-app-learning/issues | |
https://github.com/openedx/frontend-app-course-authoring/issues |
README.rst
Outdated
created when copying this template above. | ||
|
||
|
||
1. Clone your new repo: |
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.
1. Clone your new repo: | |
1. Clone the repo: |
HI @arbrandes! I've done the changes, I will be pending to any additional change |
Hi @KristinAoki! Are you able to merge this if all looks ok? Thanks! |
@arbrandes needs to approve before this can be merged because he requested changes. The PR is blocked from being merged until the changes are marked as addressed by him. |
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.
Works for me, thanks!
@johnvente 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
The README of the MFE's need to be updated because the frontend app template has been updated
Issue related: openedx/wg-frontend#169