Skip to content

Conversation

rtritto
Copy link
Contributor

@rtritto rtritto commented Mar 1, 2025

Close #161

Related Video.js plugin: https://github.com/ctd1500/videojs-hotkeys

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Copy link

vercel bot commented Mar 1, 2025

@rtritto is attempting to deploy a commit to the Vlitejs Team on Vercel.

A member of the Team first needs to authorize it.

@yoriiis
Copy link
Member

yoriiis commented Mar 1, 2025

The following lines needs to be removed

Copy link
Member

@yoriiis yoriiis left a comment

Choose a reason for hiding this comment

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

Thanks @rtritto for the contribution!

Feel free to improve the plugin sample or contributing guide if you have idea from your contribution

Copy link

vercel bot commented Mar 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
vlite ✅ Ready (Inspect) Visit Preview Mar 3, 2025 11:10am

@rtritto rtritto force-pushed the add-hotkeys-plugin branch from d8c6a11 to 3e4edad Compare March 1, 2025 18:33
@rtritto
Copy link
Contributor Author

rtritto commented Mar 1, 2025

The following lines needs to be removed

* https://github.com/vlitejs/vlite/blob/8a45ec788be3d4843ffbe23c75fd13af9424e585/src/core/vlite.ts#L210

* https://github.com/vlitejs/vlite/blob/8a45ec788be3d4843ffbe23c75fd13af9424e585/src/core/vlite.ts#L375

Should the 2 lines be adapted in hotkeys.ts or we can simple remove them?

@yoriiis
Copy link
Member

yoriiis commented Mar 1, 2025

@rtritto Please, don't force pushed, keep all commits it is more easy for the review. I will squashed them on merge or require a squash only at the end

@yoriiis
Copy link
Member

yoriiis commented Mar 1, 2025

The following lines needs to be removed

* https://github.com/vlitejs/vlite/blob/8a45ec788be3d4843ffbe23c75fd13af9424e585/src/core/vlite.ts#L210

* https://github.com/vlitejs/vlite/blob/8a45ec788be3d4843ffbe23c75fd13af9424e585/src/core/vlite.ts#L375

Should the 2 lines be adapted in hotkeys.ts or we can simple remove them?

The lines should be removed from vlite.ts and added in the plugin. You will need to adapt the this.container as you have already done

@rtritto
Copy link
Contributor Author

rtritto commented Mar 2, 2025

@rtritto Please, don't force pushed, keep all commits it is more easy for the review. I will squashed them on merge or require a squash only at the end

I will squash every time because it's clear, easy to read history and the merge uses at least 2 commits

@yoriiis
Copy link
Member

yoriiis commented Mar 2, 2025

Could you add the plugin on the demo? Will be more easy to check it on the preview. Thanks

https://github.com/vlitejs/vlite/blob/main/examples/html5/config.js

@yoriiis yoriiis force-pushed the add-hotkeys-plugin branch from dce0e92 to 7c9337c Compare March 2, 2025 23:20
@rtritto
Copy link
Contributor Author

rtritto commented Mar 3, 2025

Could you add the plugin on the demo? Will be more easy to check it on the preview. Thanks

https://github.com/vlitejs/vlite/blob/main/examples/html5/config.js

Done in the commit 4ab32a3

@yoriiis yoriiis force-pushed the add-hotkeys-plugin branch from 78c6161 to d714d6a Compare March 3, 2025 11:10
@rtritto rtritto mentioned this pull request Mar 4, 2025
6 tasks
@rtritto rtritto marked this pull request as ready for review March 4, 2025 11:00
@yoriiis
Copy link
Member

yoriiis commented Mar 4, 2025

Question about the version for this updates. Shortcut will be removed from the default behavior but I don't want to publish a major release just for this "minor" imo changes. A minor bump probably, good for you also?

@yoriiis yoriiis merged commit d168ca6 into vlitejs:main Mar 4, 2025
13 checks passed
@rtritto rtritto deleted the add-hotkeys-plugin branch March 4, 2025 11:48
@rtritto
Copy link
Contributor Author

rtritto commented Mar 4, 2025

Shortcut will be removed from the default behavior but I don't want to publish a major release just for this "minor" imo changes.

Yes, it's a breaking change.

A minor bump probably, good for you also?

Yes, it's the only alternative if you don't wont to bump the major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert shortcuts feature to a plugin
2 participants