Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(keyboardShortcut): cleanup & patch keybinds #2506

Merged
merged 8 commits into from
Aug 12, 2023
Merged

refactor(keyboardShortcut): cleanup & patch keybinds #2506

merged 8 commits into from
Aug 12, 2023

Conversation

ohitstom
Copy link
Contributor

@ohitstom ohitstom commented Aug 8, 2023

This PR removes redundant keybinds that Spotify have implemented into their own shortcut menu (ctrl + ?), whilst also fixing pre-existing ones (namely rotate navbar item which broke from libx).

The registering of these binds has also been moved to a json/object structure making it easier to expand upon both on the dev side and in a future settings menu if we were to add one.

Also moved from Spicetify.Keyboard to Spicetify.Mousetrap

(docs will need rewriting)

@ohitstom ohitstom changed the title rewrite(keyboardShortcut): fill this in please idk rewrite(keyboardShortcut): redundancy removal + callback repairs Aug 8, 2023
@ohitstom ohitstom changed the title rewrite(keyboardShortcut): redundancy removal + callback repairs rewrite(keyboardShortcut): redundancy removal & callback repairs Aug 8, 2023
@ohitstom ohitstom changed the title rewrite(keyboardShortcut): redundancy removal & callback repairs refactor(keyboardShortcut): redundancy removal & callback repairs Aug 8, 2023
@Delusoire
Copy link
Contributor

It takes a noticeable amount of time (seconds) to add/remove hundreds of elements directly to/from the DOM. All element.remove() and element.append() should happen only after vimOverlay is removed, and when all the changes to its descendants are made, we then can add it back to the DOM. With this, the whole operation becomes quasi-instant.

@ohitstom
Copy link
Contributor Author

ohitstom commented Aug 9, 2023

It takes a noticeable amount of time (seconds) to add/remove hundreds of elements directly to/from the DOM. All element.remove() and element.append() should happen only after vimOverlay is removed, and when all the changes to its descendants are made, we then can add it back to the DOM. With this, the whole operation becomes quasi-instant.

i haven't modified this behavior nor do I plan to

Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just want to check the missing shortcuts

Extensions/keyboardShortcut.js Show resolved Hide resolved
Extensions/keyboardShortcut.js Show resolved Hide resolved
Extensions/keyboardShortcut.js Show resolved Hide resolved
Extensions/keyboardShortcut.js Show resolved Hide resolved
Extensions/keyboardShortcut.js Outdated Show resolved Hide resolved
@kyrie25 kyrie25 changed the title refactor(keyboardShortcut): redundancy removal & callback repairs refactor(keyboardShortcut): cleanup & patch keybinds Aug 12, 2023
@kyrie25 kyrie25 merged commit e79414c into spicetify:master Aug 12, 2023
6 checks passed
@ohitstom ohitstom deleted the keyboard branch August 12, 2023 09:57
@ohitstom
Copy link
Contributor Author

ohitstom commented Mar 3, 2024

It takes a noticeable amount of time (seconds) to add/remove hundreds of elements directly to/from the DOM. All element.remove() and element.append() should happen only after vimOverlay is removed, and when all the changes to its descendants are made, we then can add it back to the DOM. With this, the whole operation becomes quasi-instant.

hey im looking to implement this in my new pr #2860, its my understanding that you mean instead of keeping the vimOverlay in dom and appending / removing hundreds of keys, we instead append all these children before appending to document if that makes sense?

image
image

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.

5 participants