-
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
feat: [FC-0044] Course unit page - Unit creation button logic and refactoring #831
feat: [FC-0044] Course unit page - Unit creation button logic and refactoring #831
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #831 +/- ##
==========================================
+ Coverage 89.27% 90.39% +1.11%
==========================================
Files 551 554 +3
Lines 9738 9826 +88
Branches 2099 2110 +11
==========================================
+ Hits 8694 8882 +188
+ Misses 996 910 -86
+ Partials 48 34 -14 ☔ View full report in Codecov by Sentry. |
65db5ac
to
3ac069d
Compare
Sandbox deployment successful. Sandbox LMS is available at https://pr-831-ba9a65.sandboxes.opencraft.hosting Instance Tutor config : https://axim-sandbox-build-logs-4626178240.nyc3.digitaloceanspaces.com/pr-831-ba9a65-51564656-6173465303-tutor-config.yml |
79d5a2d
to
5e9bc34
Compare
Sandbox deployment failed. Check failure logs here https://axim-sandbox-build-logs-4626178240.nyc3.digitaloceanspaces.com/pr-831-ba9a65-51564656-6178704557.log Instance Tutor config : https://axim-sandbox-build-logs-4626178240.nyc3.digitaloceanspaces.com/pr-831-ba9a65-51564656-6178704557-tutor-config.yml Please check the settings and requirements and retry deployment by updating the pull request description or pushing a new commit |
Sandbox deployment successful. Sandbox LMS is available at https://pr-831-ba9a65.sandboxes.opencraft.hosting Instance Tutor config : https://axim-sandbox-build-logs-4626178240.nyc3.digitaloceanspaces.com/pr-831-ba9a65-51564656-6189508588-tutor-config.yml |
5e9bc34
to
556001c
Compare
Sandbox deployment successful 🚀 |
… page components (#118) * feat: added modal windows for course unit page components * refactor: code refactoring * refactor: added translations * refactor: refactoring after review * refactor: after review
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.
It works, and I like the direction of the refactor. I have one nit about the renaming of TitleEditForm
to EditTitleForm
, though, but only because similar variables have not been similarly changed. (If you want to change that, totally fine, but we should probably commit to changing the other ones too.)
I don't mind the failure in coverage, at least for now.
@@ -73,6 +73,7 @@ const CourseAuthoringRoutes = () => { | |||
/> | |||
{DECODED_ROUTES.COURSE_UNIT.map((path) => ( | |||
<Route | |||
key={path} |
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.
Non-blocking, just a question. wondering how it was working before. We probably had warnings, right?
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.
Yes, a warning about the need to add a key
for the element that is being mapped
Sandbox deployment successful 🚀 |
@arbrandes Thank you! Your wishes were taken into account + test coverage increased after rolling back the renaming of variables and Sandbox deployment was successful 🚀 |
Thank you for addressing my comments! |
@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Settings
Description
This pull request is a logical continuation of optimizing the CMS of the navigation widget for course units. The second refactoring stage was carried out which will end in the next pull request to transfer the navigation widget from the MFE Learning. In addition, the functionality of a button to create a new unit for the Course unit page was implemented.
The primary features were implemented:
Issue: openedx/platform-roadmap#321
Developer notes
Design
Figma design
Testing instructions
contentstore.new_studio_mfe.use_new_unit_page
in the CMS admin panel.ENABLE_UNIT_PAGE=true
is enabled.