-
Notifications
You must be signed in to change notification settings - Fork 76
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] Unit page - Manage access modal (unit & xblocks) #901
feat: [FC-0044] Unit page - Manage access modal (unit & xblocks) #901
Conversation
Thanks for the pull request, @ihor-romaniuk! 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. |
77d3a17
to
9f3b527
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #901 +/- ##
==========================================
- Coverage 92.00% 91.98% -0.02%
==========================================
Files 612 574 -38
Lines 10746 10246 -500
Branches 2305 2234 -71
==========================================
- Hits 9887 9425 -462
+ Misses 830 794 -36
+ Partials 29 27 -2 ☔ View full report in Codecov by Sentry. |
Sandbox deployment successful 🚀 |
9f3b527
to
294962b
Compare
Hello @arbrandes @KristinAoki |
Sandbox deployment successful 🚀 |
294962b
to
a5476d1
Compare
Sandbox deployment failed 💥 |
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.
Code looks good, except possibly for one nit. Waiting for sandbox to deploy to actually test it.
src/constants.js
Outdated
@@ -49,3 +49,10 @@ export const DECODED_ROUTES = { | |||
'/container/:blockId', | |||
], | |||
}; | |||
|
|||
export const COURSE_BLOCK_NAMES = /** @type {const} */ ({ |
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.
export const COURSE_BLOCK_NAMES = /** @type {const} */ ({ | |
export const COURSE_BLOCK_NAMES = ({ |
I'm all for JSDoc, but I'm not sure I like having it sprinkled piecemeal in a file. If we go for it, we should do it for all instances. (At least on a per-file basis.)
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.
Completely agree with you.
JSDoc was deleted for this case and resolved conflicts.
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
a5476d1
to
394fcbd
Compare
Sandbox deployment successful 🚀 |
aa383fc
to
c5f3a76
Compare
Sandbox deployment failed 💥 |
Sandbox deployment successful 🚀 |
c5f3a76
to
8e145d3
Compare
Sandbox deployment successful 🚀 |
8e145d3
to
1d6fb7b
Compare
Hello @arbrandes @KristinAoki |
Sandbox deployment successful 🚀 |
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.
For the message.js
files that you moved, can you add descriptions to each of the messages?
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.
Looks great in general, but it would be good to add message descriptions, where apropriate. When done, this is good to merge, so I'll approve preemptively.
@KristinAoki @arbrandes All new messages were covered by descriptions and it's ready for the final check. |
Sandbox deployment failed 💥 |
@arbrandes This is ready to be merged! |
@ihor-romaniuk 🎉 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 introduces a crucial feature to our platform: the ability to manage content visibility, specifically access rights, for both course units and course XBlocks. We empower educators to finely control access rights.
Reuse the
ConfigureModal
component for consistency across the application.Issue: openedx/platform-roadmap#321
Depends on:
Design
https://www.figma.com/file/YeKFwSpyLaJFDs3f3p3TSa/Studio-1%3A1-mock-ups?node-id=599%3A23595
Testing instructions
contentstore.new_studio_mfe.use_new_unit_page
in CMS admin panel.