Render native add-component strip for generic container XBlocks#3058
Render native add-component strip for generic container XBlocks#3058salman2013 wants to merge 2 commits into
Conversation
|
Thanks for the pull request, @salman2013! 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. DetailsWhere 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3058 +/- ##
========================================
Coverage 95.52% 95.52%
========================================
Files 1393 1393
Lines 32969 33544 +575
Branches 7592 7899 +307
========================================
+ Hits 31493 32043 +550
- Misses 1408 1432 +24
- Partials 68 69 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The general approach on both this PR and the openedx-platform PR look good to me! When you're ready, can you make a sandbox for testing? You can make it on this PR, and use tutor configuration to point the backend at your openedx-platform branch: |
|
@kdmccormick The sandbox has been created successfully if you could get a chance to take a look at it. |
|
@bradenmacdonald Could you also take a look into this PR. thanks. |
|
@salman2013 Why are you closing and re-opening this PR so many times? Do you not have the "re-run checks" button in the UI on this page ?
|
| AddComponentButton.defaultProps = { | ||
| beta: false, | ||
| disabled: false, | ||
| disabledReason: null, |
There was a problem hiding this comment.
defaultProps is deprecated - please don't add any new defaults to defaultProps. Instead, use ES6 function defaults and remove the whole defaultProps entirely.
| onClick: PropTypes.func.isRequired, | ||
| beta: PropTypes.bool, | ||
| disabled: PropTypes.bool, | ||
| disabledReason: PropTypes.string, |
There was a problem hiding this comment.
This is OK, but we prefer if you can convert this file to TypeScript and remove propTypes altogether while you're changing this file anyways.
| ComponentModalView.defaultProps = { | ||
| isRequestedModalView: false, | ||
| disabled: false, | ||
| disabledReason: null, |
There was a problem hiding this comment.
Same here: do not add defaultProps.
| export interface AddComponentProps { | ||
| isSplitTestType?: boolean; | ||
| isUnitVerticalType?: boolean; | ||
| isGenericContainerType?: boolean; |
There was a problem hiding this comment.
This is kind of vague. Please add a /** JSDoc comment */ explaining what this parameter is for.
| isSplitTestType={isSplitTestType} | ||
| isUnitVerticalType={isUnitVerticalType} | ||
| isProblemBankType={isProblemBankType} | ||
| isGenericContainerType={isGenericContainerType} |
There was a problem hiding this comment.
We should really just pass the containerType in, instead of isSplitTestType + isUnitVerticalType + isProblemBankType + isGenericContainerType. Having all these booleans is verbose and messy.
| updateQueryPendingStatus, | ||
| } from './data/slice'; | ||
|
|
||
| export const useCourseUnit = ({ |
There was a problem hiding this comment.
Nit: this can be fixed in a separate PR, but if this hook/component is used for split test, problem bank, and many other things, it shouldn't be called "CourseUnit" and should be called "CourseComponentContainer" or something more generic.

Description
Note: The changes in this PR is generated by Claude Sonet 4.6, reviewed by Salman. Based on the implementation plan suggested on the original ticket (#2620)
Details for the original bug can be found here: #2620
The backend (openedx-platform) dependent PR for this change: openedx/openedx-platform#38636
Settings