Skip to content

Conversation

@farhan
Copy link
Contributor

@farhan farhan commented Apr 24, 2025

Ticket: #36413

  • In this PR we are moving Video Block JS into xmodule/assets as discussed in this readme
  • Replacing RequireJS with ES6 import/export

JS files refactoring details:

  • Reviewer can open the second or next commits to review the changes as the first commit contains the old code.
  • diffchecker can be used (on real-time-diff) to see the accurate refactoring changes.
# Source JS File (RequireJS pattern) Destination JS File (Vanilla JS)
1 00_i18n.js 00_i18n.js
2 00_async_process.js 00_async_process.js
3 09_poster.js 09_poster.js
4 035_video_accessible_menu.js 035_video_accessible_menu.js
5 00_iterator.js 00_iterator.js
6 09_skip_control.js 09_skip_control.js
7 09_play_placeholder.js 09_play_placeholder.js
8 00_component.js 00_component.js
9 09_play_skip_control.js 09_play_skip_control.js
10 09_play_pause_control.js 09_play_pause_control.js
11 036_video_social_sharing.js 036_video_social_sharing.js
12 00_video_storage.js 00_video_storage.js
13 10_commands.js 10_commands.js
14 09_bumper.js 09_bumper.js
15 09_events_bumper_plugin.js 09_events_bumper_plugin.js
16 00_sjson.js 00_sjson.js
17 08_video_auto_advance_control.js 08_video_auto_advance_control.js
18 09_save_state_plugin.js 09_save_state_plugin.js
19 025_focus_grabber.js 025_focus_grabber.js
20 02_html5_hls_video.js 02_html5_hls_video.js
21 09_events_plugin.js 09_events_plugin.js
22 04_video_control.js 04_video_control.js
23 05_video_quality_control.js 05_video_quality_control.js
24 00_resizer.js 00_resizer.js
25 09_completion.js 09_completion.js
26 10_main.js 10_main.js
27 037_video_transcript_feedback.js 037_video_transcript_feedback.js
28 04_video_full_screen.js 04_video_full_screen.js
29 06_video_progress_slider.js 06_video_progress_slider.js
30 02_html5_video.js 02_html5_video.js
31 08_video_speed_control.js 08_video_speed_control.js
32 07_video_volume_control.js 07_video_volume_control.js
33 095_video_context_menu.js 095_video_context_menu.js
34 01_initialize.js 01_initialize.js
35 03_video_player.js 03_video_player.js
36 09_video_caption.js 09_video_caption.js
37 time.js time.js

How to test:

  • Check out the branch
  • run npm run webpack-dev
  • Do Hard cache reload of the browser

Old archived PR

@farhan farhan force-pushed the farhan/move-video-js-to-assets branch 9 times, most recently from 1419e98 to 890e51f Compare April 29, 2025 13:07
@farhan farhan force-pushed the farhan/move-video-js-to-assets branch from e1d1c03 to 61d0116 Compare May 2, 2025 18:49
@farhan farhan force-pushed the farhan/original-js-code branch from d26b6c7 to 613e9f6 Compare May 2, 2025 18:50
@farhan farhan added the create-sandbox open-craft-grove should create a sandbox environment from this PR label May 2, 2025
@farhan farhan changed the base branch from farhan/original-js-code to master May 5, 2025 05:47
@farhan farhan marked this pull request as ready for review May 5, 2025 05:49
@farhan farhan closed this May 5, 2025
@farhan farhan reopened this May 5, 2025
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@farhan farhan force-pushed the farhan/move-video-js-to-assets branch 2 times, most recently from 0802d3f to 0b85563 Compare May 5, 2025 09:57
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@bradenmacdonald
Copy link
Contributor

Wow, great to see this. Question: why are the old .js files not deleted as part of this PR?

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@farhan farhan requested a review from ahmad-arbisoft May 6, 2025 13:55
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Copy link

@ahmad-arbisoft ahmad-arbisoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@farhan
Copy link
Contributor Author

farhan commented May 7, 2025

Wow, great to see this. Question: why are the old .js files not deleted as part of this PR?

@bradenmacdonald
Thanks!
I was thinking to delete old JS files in separate PR, I think it makes better sense to delete them within this PR.
On the way.

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@farhan farhan force-pushed the farhan/move-video-js-to-assets branch from 4f66eb1 to 0d2a43f Compare September 30, 2025 10:07
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@farhan farhan force-pushed the farhan/move-video-js-to-assets branch from b1a2c4c to 9b4a963 Compare October 3, 2025 12:45
- Move Video Block JS files from xmodule/js/src/video/ to xmodule/assets/video/public/js/
- Update JavaScript files from  RequireJS to ES6 import/export
@farhan farhan force-pushed the farhan/move-video-js-to-assets branch from 9b4a963 to cf26705 Compare October 3, 2025 12:51
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

* fix: enable and fix webpack karma js tests for video block
@farhan farhan force-pushed the farhan/move-video-js-to-assets branch from cf26705 to d379540 Compare October 4, 2025 06:20
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Copy link
Contributor

@salman2013 salman2013 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick kdmccormick removed the blocked by other work PR cannot be finished until other work is complete label Oct 6, 2025
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 🎉

Before merging, can you confirm that you have manually tested both the CMS and LMS views of the video block?

Co-authored-by: Kyle McCormick <kyle@axim.org>
@farhan farhan closed this Oct 6, 2025
@farhan farhan reopened this Oct 6, 2025
@farhan
Copy link
Contributor Author

farhan commented Oct 6, 2025

@kdmccormick
Yes we have tested the PR manually.

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@farhan farhan merged commit 5c759f1 into master Oct 7, 2025
63 of 65 checks passed
@farhan farhan deleted the farhan/move-video-js-to-assets branch October 7, 2025 14:01
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

create-sandbox open-craft-grove should create a sandbox environment from this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Video Block | Replace RequireJS with ES6 import/export #36527

9 participants