Feature PR | Extract Video Block from edx-platform#168
Conversation
xblocks_contrib/common/utils.py
Outdated
| from django.conf import settings | ||
|
|
||
|
|
||
| def get_resource_url(xblock, path, package_scope=None): |
There was a problem hiding this comment.
Ideally every xblock should be as independent and we should define this under the xblock's own class.
There was a problem hiding this comment.
It's to follow DRY.
If there are some core/complex utils that will follow the layered architecture we should keep them separate in 1 hierarchy above. Like we already did here
XBlock will stay independent, you can consider it like we are adding some thirdpary pypi package.
There was a problem hiding this comment.
I agree with applying DRY for stable and reusable abstractions. However, not all XBlocks currently use the bundled JS approach. I believe 3 out of 8 xblocks are using, so extracting a shared loader at a higher hierarchy might introduce unnecessary coupling
I think this util has most of the functionality related to xmlMixin which we intend to improve in future.
There was a problem hiding this comment.
It will work as well.
@kdmccormick could you please take a look at this method? Do you think we’re moving any XBlock-related utility methods into xblock/utils that are tightly coupled with the openedx-platform runtime? or what could be the best approach?
Note: I don't know the amount of such methods
| @@ -0,0 +1,1257 @@ | |||
| /* CSS for VideoBlock */ | |||
There was a problem hiding this comment.
Could you figure out why this file change is showing as we have already merged lift and shift video code and this css file has merged.
Ref : https://github.com/openedx/xblocks-contrib/pull/142/changes#diff-4e6584fac3887788f6f20e53793f005629eb437d2395a3a5a7c182f7fcb8f9d1
There was a problem hiding this comment.
This PR has been merged into this feature branch PR, so it's normal
| @@ -0,0 +1,1257 @@ | |||
| /* CSS for VideoBlock */ | |||
There was a problem hiding this comment.
This file and all other js files has already reviewed and merge in this PR
Why this is showing again in this feature branch?
There was a problem hiding this comment.
PR you shared merging the changes into this feature branch and now we will keep all the changes altogether to main.
- It will maintain PR's history
- It will give us green test checks on the PR while merging into main
farhan
force-pushed
the
farhan/video-block
branch
from
f2009b9 to
6938d26
Compare
farhan
force-pushed
the
farhan/video-block
branch
from
20f6838 to
d49b34f
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 can easily view the next commit to view the changes done in xblocks-contrib while shifting code from the edx-platform
* fix: fix quality check for video block code * chore: update package.lock.json
farhan
force-pushed
the
farhan/video-block
branch
from
d49b34f to
7eae34b
Compare
3 participants
Extract Video Block from edx-platform
Relevant test cases PR on edx-platform:
USE_EXTRACTED_VIDEO_BLOCK=True [Sandbox is up on above PR, but can't render video block as of un-availability of public assets]
Run Test cases on extracted video block | USE_EXTRACTED_VIDEO_BLOCK=True openedx-platform#37996
USE_EXTRACTED_VIDEO_BLOCK=False
Run Test cases on extracted video block | USE_EXTRACTED_VIDEO_BLOCK=False openedx-platform#38047
Following PR's have been merged in this PR:
#141
#142
#167
#143
Other relevant PR's:
#144 [test_video, need code review]
#103 [Karma test cases, need reconfiguring]