fix: Rename optimistic update in children containers - #2141
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! 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. |
|
@ChrisChV let me know when you've fixed the cyclic import test error, and then I can test and review. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2141 +/- ##
==========================================
- Coverage 94.10% 94.09% -0.01%
==========================================
Files 1164 1164
Lines 24495 24507 +12
Branches 5310 5201 -109
==========================================
+ Hits 23050 23060 +10
- Misses 1377 1379 +2
Partials 68 68 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@bradenmacdonald Done! |
|
@ChrisChV It's mostly working, but I'm still seeing a weird flash of the old name for a second. optimistic.error.mov |
|
@bradenmacdonald I tried it again, and I don't see the flash. But it has happened to me that I have had to start my tutor again after changing branches. Can you try that to see if the flickering continues? https://www.loom.com/share/c221323489574697ab20937065ef2b0d?sid=f9cac243-41c4-4185-a547-4c3ab310b79f |
|
@ChrisChV Weird. I restarted and rebuilt everything and it was still happening. Then I tried making some changes and now it's working! Not really sure what's going on. |
| // In some cases, the title is already available, but it's retrieved in a list of containers. | ||
| // In these cases, it's necessary to use this `ContainerEditableTitle` for the optimistic update to work. | ||
| // By using `placeHolderText`, we can give the illusion that the data | ||
| // has already been loaded before using the real data. | ||
| placeHolderText?: string; |
There was a problem hiding this comment.
I don't really understand why we need this.
When I removed it in 8576925 and tested, it's still working fine (the optimistic update works without it).
it's retrieved in a list of containers.
Where is that happening? Before you added this placeHolderText, the only place I see this component getting the title from is useContainer(containerId) which is not a list of containers. Maybe we should be getting it from a list though, as it may be a little inefficient loading each container separately if there's a lot in the parent.
There was a problem hiding this comment.
Where is that happening?
In LibraryContainerChildren, you get the containers in a list using useContainerChildren
The optimistic update is implemented by overwriting the data of the useContainer query. I feel like doing the same for the useContainerChildren query is going to be more complicated.
as it may be a little inefficient loading each container separately if there's a lot in the parent
This is seen visually if we do not use the placeHolderText:
https://www.loom.com/share/0090987109b44902809f55b15271090d?sid=3666ee1c-02e1-4655-8713-d3aca877454e
If you'd like, I can try implementing an optimistic update overriding useContainerChildren. This problem also exists in the component listing on the unit page, it is a separate implementation, but follows the same logic.
There was a problem hiding this comment.
If you'd like, I can try implementing an optimistic update overriding useContainerChildren. This problem also exists in the component listing on the unit page, it is a separate implementation, but follows the same logic.
I think that would be better if you don't mind giving it a try!
There was a problem hiding this comment.
It works! It's ready for a review
|
I also found an annoying bug: if you are editing a title and you double-click in the Screen.Recording.2025-06-18.at.11.56.12.AM.mov |
Fixed: 78dc044 |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Great job! I like this approach more. Thanks for refactoring. I tested and it's working well now.
Please just add a comment, and also before merging please rebase or merge master and then manually test, just to make sure it's working OK with the Paragon 23 changes on master.
| * Use this mutation to update the fields of a container in a library | ||
| */ | ||
| export const useUpdateContainer = (containerId: string) => { | ||
| export const useUpdateContainer = (containerId: string, affectedParentContainerId?: string) => { |
There was a problem hiding this comment.
Could you please just update the JSDoc comment about to explain what the new affectedParentContainerId parameter is doing exactly?
3 participants
Description
Supporting information
Testing instructions
Other information