Extract Video Block from edx-platform | Update the shifted Video XBlock code#142
Extract Video Block from edx-platform | Update the shifted Video XBlock code#142farhan wants to merge 15 commits intofarhan/video-block-lift-and-siftfrom
Conversation
102abb6 to
6a3edee
Compare
9415c91 to
cd2263a
Compare
|
Upon taking a skim through it, I don't see any issues. @farhan can you have a team member review, and merge it if it looks good to both of you? |
d684cf3 to
0c374ea
Compare
fc8cafa to
302af97
Compare
xblocks_contrib/video/.gitignore
Outdated
| @@ -0,0 +1,3 @@ | |||
| node_modules/* | |||
There was a problem hiding this comment.
Lets move this to root .gitignore
| "scripts": { | ||
| "build": "webpack --config webpack.prod.config.js", | ||
| "build-dev": "webpack --config webpack.dev.config.js", | ||
| "test": "echo \"Error: no test specified\" && exit 1" |
There was a problem hiding this comment.
We need to make our root package.json have these commands for it to run.
| EmptyDataRawMixin, XmlMixin, EditingMixin, XModuleToXBlockMixin, | ||
| ResourceTemplates, XModuleMixin, LicenseMixin): | ||
| @XBlock.wants('settings', 'completion', 'request_cache', 'video_config') | ||
| @XBlock.needs('i18n', 'user') |
There was a problem hiding this comment.
what made us move i18n from wants to needs?
There was a problem hiding this comment.
I think it should be as its availability check doesn't exist on its usage.
It also in sync up with all other blocks of xblocks-contrib.
xblocks_contrib/video/video_utils.py
Outdated
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, thanks for pointing it
xblocks_contrib/video/package.json
Outdated
| "draggabilly": "^3.0.0", | ||
| "edx-ui-toolkit": "1.8.7", | ||
| "hls.js": "0.14.17", | ||
| "underscore": "^1.13.7", | ||
| "webpack-merge": "^6.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "clean-webpack-plugin": "^4.0.0", | ||
| "webpack": "^5.99.8", | ||
| "webpack-cli": "^6.0.1", | ||
| "webpack-manifest-plugin": "^5.0.1" |
There was a problem hiding this comment.
Are these packages versions inline with edx-platform/package.json?
There was a problem hiding this comment.
There were, going to update them again as changed in following PR
openedx/openedx-platform#37961
| ] | ||
| }; | ||
|
|
||
| module.exports = config |
There was a problem hiding this comment.
we need to confirm that the bundle produced by this is exactly the same as that of edx-platform's one. Right now I can see alot of differences between both
| tabindex="-1" | ||
| > | ||
| data-package-name="xblocks-contrib" | ||
| data-test-id="1"> |
There was a problem hiding this comment.
Using it for the testing on the sanbox to check the versions, removed ➖
xblocks_contrib/video/video.py
Outdated
| fragment.add_css(loader.load_unicode("static/css/video.css")) | ||
| fragment.add_javascript_url( | ||
| self.runtime.local_resource_url(self, self._video_js_resource_path()) | ||
| ) | ||
| fragment.initialize_js("Video") |
There was a problem hiding this comment.
Shouldn't we load both from runtime?
| fragment.add_css(loader.load_unicode("static/css/video.css")) | |
| fragment.add_javascript_url( | |
| self.runtime.local_resource_url(self, self._video_js_resource_path()) | |
| ) | |
| fragment.initialize_js("Video") | |
| fragment.add_css(self.runtime.local_resource_url(self, "public/css/video.css")) | |
| fragment.add_javascript_url( | |
| self.runtime.local_resource_url(self, self._video_js_resource_path()) | |
| ) | |
| fragment.initialize_js("Video") |
| return metadata_fields | ||
|
|
||
| @property | ||
| def editable_metadata_fields(self): |
There was a problem hiding this comment.
we should've kept it in video block to minimize change.
0c374ea to
de657bb
Compare
This commit contains original code copied from edx-platform. Purpose of keeping the first commit with original code is to get help in the review process. Reviewer just needs to open the second commit and code changes will be clearly visible.
ffa33f7 to
8155f17
Compare
Ticket
Description:
Code has been updated in this PR to make Video Block independent from the edx-platform.
edx-platformtest cases PRedx-platform) PRExtracted Video XBlock Sandbox to test the whole extraction: sandbox-pr
Testing Notes:
Extracted Video XBlock can be manually tested on the local machine setup up.
Known Issues/Pending Stories in Video XBlock Extraction:
edx-platformprior work:Following is the prior work already done in the
edx-platformto decouple Video Block from platform by introducingVideoConfigServiceopenedx/openedx-platform#37829
openedx/openedx-platform#37809
openedx/openedx-platform#37808
openedx/openedx-platform#37793
openedx/openedx-platform#37779
openedx/openedx-platform#37776
openedx/openedx-platform#37635
openedx/openedx-platform#37631
openedx/openedx-platform#37459