feat: implement scrolling to XBlocks when browsing metrics#24
Conversation
|
Thanks for the pull request, @tecoholic! 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. |
| "react": "^17.0.2", | ||
| "react-dom": "^17.0.2", | ||
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1", |
There was a problem hiding this comment.
Note: After a recent rebase on Authoring MFE master, the MFE wouldn't load at all until I updated the React Version to match the Authoring MFE.
| "scripts": { | ||
| "preinstall": "npm run build", | ||
| "build": "fedx-scripts babel -x .js,.jsx,.ts,.tsx src --out-dir dist --ignore **/*.test.jsx,**/*.test.js,**/setupTest.js", | ||
| "build": "fedx-scripts babel -x .js,.jsx,.ts,.tsx src --out-dir dist --ignore **/*.test.jsx,**/*.test.js,**/setupTest.js && cp src/*.css dist/", |
There was a problem hiding this comment.
I just tacked on a cp to copy the CSS file. Since we are running babel directly without webpack, there is not css-loader and didn't want to spend too much time on this.
There was a problem hiding this comment.
I think this is entirely fair since we have no need for CSS here and the future of paragon is to use CSS variables.
| "scripts": { | ||
| "preinstall": "npm run build", | ||
| "build": "fedx-scripts babel -x .js,.jsx,.ts,.tsx src --out-dir dist --ignore **/*.test.jsx,**/*.test.js,**/setupTest.js", | ||
| "build": "fedx-scripts babel -x .js,.jsx,.ts,.tsx src --out-dir dist --ignore **/*.test.jsx,**/*.test.js,**/setupTest.js && cp src/*.css dist/", |
There was a problem hiding this comment.
I think this is entirely fair since we have no need for CSS here and the future of paragon is to use CSS variables.
src/components/AspectsSidebar.tsx
Outdated
| // can be used only with the provider. So here the context is first checked before | ||
| // the hook is called. | ||
| const ctx = React.useContext(IframeContext); | ||
| const {sendMessageToIframe} = !!ctx ? useIframe() : {}; |
There was a problem hiding this comment.
You should not have an optional hook.
i.e. here the hook only runs if ctx is defined, but won't run otherwise. This can lead to errors with hooks.
Use something like the following:
const iframe = useIframe();
const {sendMessageToIframe} = !!ctx ? iframe : {};There was a problem hiding this comment.
@xitij2000 I knew this was pretty hacky, but calling useIframe() unconditionally breaks the component on CourseOutline page as there is not IframeContext.
useIframe must be used within an IframeProvider
Call Stack
useIframe
src/generic/hooks/context/hooks.tsx:19:11
CourseContentList
frontend-plugin-aspects/dist/components/AspectsSidebar.js:60:104
renderWithHooks
node_modules/react-dom/cjs/react-dom.development.js:15486:18
mountIndeterminateComponent
node_modules/react-dom/cjs/react-dom.development.js:20098:13
beginWork
node_modules/react-dom/cjs/react-dom.development.js:21621:16
I think I might need to call useIframe in the UnitPageSidebar (where I am sure the IframeContext exists) and pass it down as props. Kindly let me know if there is a better way to do this.
There was a problem hiding this comment.
I think that might be the way to go. If you have a hook that should be conditional, it's better to put it in a separate component. i.e. in this case you can have sendMessageToIFrame as an optional prop in AspectsSidebar and then have a UnitPageSidebar that uses AspectsSidebar and uses the hook to populate that prop, and an OutlineSidebar that leaves it as null.
There was a problem hiding this comment.
@xitij2000 Thank you. It worked out pretty nicely in 6a7e633
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
=========================================
+ Coverage 0 16.61% +16.61%
=========================================
Files 0 15 +15
Lines 0 331 +331
Branches 0 66 +66
=========================================
+ Hits 0 55 +55
- Misses 0 276 +276 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This commit also refactors a lot of the implementation of the sidebar and adds stickiness so that the metrics and the components are both visible when scrolling.
- Stale dashboard when navigating back from specific component dashbord - text "0" when there are no elements in a ContentList
482fca4 to
61b0e4f
Compare
farhaanbukhsh
left a comment
There was a problem hiding this comment.
👍
- ✅ I tested this on the sandbox
- ✅ I read through the code
- ❌ I checked for accessibility issues
- ❌ Includes documentation
- ❌ I made sure any change in configuration variables is reflected in the corresponding client's
configuration-securerepository.
Changelist
1. Styling
2. Unit Page
3. Course Outline Sidebar
Screen capture of the scrolling in action
Screencast_20250414_143034.webm
Testing Instructions
This PR depends on a number of custom branches, so set them all up in the tutor environment before testing.
edx-platformas tutor mount and checkout to this PR branchtutor-contrib-aspectsfrom this PR branchplatform-plugin-aspectsfrom this PR branch and add it as a tutor-mount.frontend-app-authoringand checkout to this branch and add it as a tutor mount so that the authoring-dev image is created.frontend-app-authoringclone this repo and checkout the PR. This is needed as thefrontend-app-authoringis being bind mounted and we need access to this plugin's PR branch in the mounted volume.npm install ./frontend-plugin-aspectsto install the plugin for the Authoring MFE.env.config.jsxto match the following:tutor dev launchand wait for everything to be online. Import the democourse if it's not done already.Course Unit Analyticsin this PR description to enable it for the test course.