feat: handle duplicate children in container pages [FC-0112]#2584
feat: handle duplicate children in container pages [FC-0112]#2584ChrisChV merged 9 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @navinkarkera! 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 #2584 +/- ##
========================================
Coverage 94.81% 94.81%
========================================
Files 1225 1225
Lines 27481 27514 +33
Branches 6187 6037 -150
========================================
+ Hits 26056 26088 +32
- Misses 1367 1368 +1
Partials 58 58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0b97bc8 to
7a536e9
Compare
|
|
||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| axiosMock.restore(); | ||
| }); |
There was a problem hiding this comment.
| afterEach(() => { | |
| jest.clearAllMocks(); | |
| axiosMock.restore(); | |
| }); |
This is not necessary - initializeMocks() already calls clearAllMocks() for you, and it re-creates the axiosMock each time.
| * Fetch a library container's children's metadata. | ||
| */ | ||
| export async function getLibraryContainerChildren( | ||
| export async function getLibraryContainerChildren<T>( |
There was a problem hiding this comment.
Instead of T can you call this ChildType or something more obvious, and make it default to LibraryBlockMetadata[] | Container[] ?
| export async function getLibraryContainerChildren<T>( | |
| export async function getLibraryContainerChildren<ChildType = LibraryBlockMetadata | Container>( |
Same with useContainerChildren - just looking at the hook it's not obvious what the T type parameter is for, and it should have a sensible default type.
There was a problem hiding this comment.
That looks better. Thanks!
0229f95 to
b97b548
Compare
b97b548 to
4af212b
Compare
rpenido
left a comment
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @navinkarkera!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
ChrisChV
left a comment
There was a problem hiding this comment.
@navinkarkera Looks good! I commented on some nits
Adds index number to url and allow selecting one instance and removing it without affecting other instances in the same page
4af212b to
5d929ae
Compare
|
@ChrisChV This is ready to be merged. |
…#2584) If we have duplicate container or component in parent page in library, clicking on one of them selects both and removing any one from the parent blocks removes all instances. This PR handles duplicates by including index/order_number of each child component in the url and using it to exclude a single instance and update parent structure. (cherry picked from commit 75ae9d5)
Description
If we have duplicate container or component in parent page in library, clicking on one of them selects both and removing any one from the parent blocks removes all instances.
This PR handles duplicates by including index/order_number of each child component in the url and using it to exclude a single instance and update parent structure.
Useful information to include:
Supporting information
Private-ref: https://tasks.opencraft.com/browse/FAL-4270Testing instructions
Other information
Include anything else that will help reviewers and consumers understand the change.
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypes,defaultProps, andinjectIntlpatterns are not used in any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'