feat: container info sidebar and add sidebar updates#2830
feat: container info sidebar and add sidebar updates#2830navinkarkera wants to merge 50 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. |
5f8ac0b to
feba274
Compare
9f4a330 to
4f29a90
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2830 +/- ##
==========================================
+ Coverage 95.18% 95.21% +0.03%
==========================================
Files 1319 1326 +7
Lines 30022 30355 +333
Branches 6748 6861 +113
==========================================
+ Hits 28575 28902 +327
- Misses 1378 1384 +6
Partials 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3ceae1c to
5590401
Compare
2cc17aa to
e685ab3
Compare
rpenido
left a comment
There was a problem hiding this comment.
First of all, awesome work here @navinkarkera!
Thanks for all the refactors! After this PR, we will be way closer to saying goodbye to our redux store.
I asked a few clarifying questions about our invalidation strategy.
Everything worked as expected, except for a small bug with the taxonomy button on the header.
| iconAs={EditIcon} | ||
| onClick={onClickEdit} | ||
| onClick={onEditClick} | ||
| // @ts-ignore |
There was a problem hiding this comment.
I think this was fixed upstream
| // @ts-ignore |
src/course-outline/data/api.ts
Outdated
| * @param {string} itemId | ||
| * @returns {Promise<Object>} |
There was a problem hiding this comment.
I don't think we need this anymore.
| * @param {string} itemId | |
| * @returns {Promise<Object>} |
| await callback?.(data.locator, variables.parentLocator); | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(variables.parentLocator) }); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseDetails(getCourseKey(data.locator)), |
There was a problem hiding this comment.
Do we need to invalidate the courseDetails here? Do you know which data it sync?
Maybe this is fixing a sync issue by its side effects: refetching the course details would cause a re-render, which will resync the data.
It would be preferable that we find the root cause, or, if it is starting to become complicated, just invalidate the whole course with courseOutlineQueryKeys.course(getCourseKey(data.locator)),
There was a problem hiding this comment.
@rpenido We invalidate courseDetails to update the course sync status which is displayed in status bar in course outline. I am not sure what you mean by re-render but this api is quiet small and only used in fetching course metadata (not the whole course outline).
src/course-outline/data/apiHooks.ts
Outdated
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseDetails(courseId) }); | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(variables.itemId) }); |
There was a problem hiding this comment.
We shouldn't need to invalidate anything besides containerComparisonQueryKeys.course(courseId) here, since they all have the same predicate.
Could you check it?
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseDetails(courseId) }); | |
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(variables.itemId) }); |
There was a problem hiding this comment.
No, courseOutlineQueryKeys.courseDetails is different from courseOutlineQueryKeys.course and courseOutlineQueryKeys.courseItem. They are related to fetching outline data while course details will just fetch course metadata.
If we invalidate courseOutlineQueryKeys.course(courseId), it will invalidate all course item queries unnecessarily. We only need to invalidate the item itself and the course metadata to update status bar.
There was a problem hiding this comment.
My bad! I just noted that containerComparisonQueryKeys.course was a different key.
| mutationFn: publishCourseItem, | ||
| onSettled: async (_data, _err, itemId) => { | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(itemId) }); | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseDetails(getCourseKey(itemId)) }); |
There was a problem hiding this comment.
Same thing with the courseDetail here
| /** | ||
| * Creates a resizable box that can be dragged to resize the width from the left side. | ||
| */ | ||
| export const ResizableBox = ({ |
| {/* eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex, jsx-a11y/no-static-element-interactions */} | ||
| <div className="resizable-handle" onMouseDown={onMouseDown} /> |
There was a problem hiding this comment.
I think it would be nice to add a button/handler here. I just noticed that the sidebar was resizable after looking at the code.
Also, it should help with accessibility.
This could be discussed upstream and implemented in a follow-up task if needed.
There was a problem hiding this comment.
Good point. However does the resizing really matter in accessibility?
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseItemId(contentId), | ||
| }); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseDetails(courseKey), | ||
| }); |
src/CourseAuthoringContext.tsx
Outdated
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseItemId(currentSelection?.sectionId), | ||
| }); |
There was a problem hiding this comment.
Why are we invalidating a section when opening a unit here?
There was a problem hiding this comment.
It was used to invalidate parent data after a new unit is created but not needed anymore as it is handled in the source.
| const showNewSidebar = getConfig().ENABLE_COURSE_OUTLINE_NEW_DESIGN?.toString().toLowerCase() === 'true'; | ||
| if (showNewSidebar) { | ||
| setCurrentPageKey('align', cardId); | ||
| setCurrentPageKey('align'); |
There was a problem hiding this comment.
Clicking on the taxonomy tag on the card header is crashing here:
TypeError
can't access property "component", pages[currentPageKey] is undefined
Call Stack
Sidebar
src/generic/sidebar/Sidebar.tsx:76:28
renderWithHooks
node_modules/react-dom/cjs/react-dom.development.js:15486:27
updateFunctionComponent
node_modules/react-dom/cjs/react-dom.development.js:19613:20
beginWork
node_modules/react-dom/cjs/react-dom.development.js:21636:16
callCallback
node_modules/react-dom/cjs/react-dom.development.js:4164:14
invokeGuardedCallbackDev
node_modules/react-dom/cjs/react-dom.development.js:4213:16
invokeGuardedCallback
node_modules/react-dom/cjs/react-dom.development.js:4277:31
beginWork$1
node_modules/react-dom/cjs/react-dom.development.js:27486:28
performUnitOfWork
node_modules/react-dom/cjs/react-dom.development.js:26592:12
workLoopSync
node_modules/react-dom/cjs/react-dom.development.js:26501:22
There was a problem hiding this comment.
@rpenido This was a weird one. My guess is that getConfig doesn't have all values set at the top level (initial render), so it decides that taxonomy is not enabled and so align page is not included in sidebar.
I had to update sidebar page provider to set the pages value inside OutlineSidebarPagesProvider which is component and so getConfig returns correct values and the align page is included. See 112b256
But this change now forces using OutlineSidebarPagesProvider and also the user of this provider has to use pages props to update sidebar pages in plugin.
There was a problem hiding this comment.
Good catch!
I don't think that we need this pages props here.
A plugin could override it by nesting another Provider, the way is shown in the comment before the context.
And that way, we can have more control at the plugin to also remove pages, change order, etc..
There was a problem hiding this comment.
Good catch @navinkarkera!
I would prefer that we do not use the pages prop, so we could have more flexibility (removing pages, changing order, etc..) on the plugin end.
I ported your fix (removing the props) here: 10b24e8
93f0d3a to
102034e
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
Just left one comment about removing the pages props from the provider.
Description
screencast.mp4
Following items are not implemented:
Apologies for the huge number of changes, could be help it due to migrating parts to react-query
Supporting information
Testing 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).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'