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

Add support for mod key to be dynamic based on Mac/Windows #715

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lb-
Copy link
Contributor

@lb- lb- commented Aug 20, 2023

  • Adds mod to the defaultSchema which will resolve to Meta on a MacOS like device and Control on Windows like
  • Uses mod to be used as a key filter modifier for either metaKey or ctrlKey based on the resolved value
  • Add unit tests and documentation for the mod key, and an example to the slideshow page
  • Closes Feature Request - Cross-platform "modify" for keydown filter. #654

Questions

  1. Is it ok we are using keyMappings to store the mod resolved value, as this will not be able to be used as a standalone key. Maybe we store it as mod but then use it as keyup.$mod+s->something#save for a modifier, but means it can still be used as a single press key via keyup.mod->something#info.
  2. Is it ok that we are resolving the mod key at import time? should we make this a function instead? maybe a standalone schema function. Maybe we do something like this...
      modKeyForScope: () => /* ... platform... */ ? "Meta" : "Control",
      keyMappings: {
        enter: "Enter",
        // ...
    
  3. Should I add functionality to the testing scaffolding to allow for modifying the schema before a test is run? Or is there any better way to do this in my tests.

@lb-
Copy link
Contributor Author

lb- commented Aug 20, 2023

It appears that the CI is running as non-Mac but when running Chrome headless locally it runs as mac potentially.

I will have to review this PR a bit later and see if I can find a better way to test this feature in unit tests.

@lb- lb- force-pushed the feature/654-mod-keyboard-modifier-support branch from cd03c7e to 9f433fb Compare August 21, 2023 10:06
@lb-
Copy link
Contributor Author

lb- commented Aug 21, 2023

Have adjusted the tests to provide better coverage of the different platforms, added some more tests and ensure the CI passes.

I have also added some questions about this PR to check the approach taken.

@lb- lb- force-pushed the feature/654-mod-keyboard-modifier-support branch from 9f433fb to 6b3291c Compare August 21, 2023 10:36
@lb-
Copy link
Contributor Author

lb- commented Aug 21, 2023

Here is a branch comparison with one of the different approaches described above, instead of mod being inside schema.keyMappings it uses a memoized function to resolve the mod key.

This approach will give a bit more control to any overrides of the schema and avoids confusion where mod cannot be used as a standalone key even though it was going to be in keyMappings.

It does not solve for existing schema mod key mappings in the wild though which would no longer work with this approach. However, other schema usage could easily update their standalone mod usage to something like $mod.

https://github.com/hotwired/stimulus/compare/main...lb-:feature/654-mod-keyboard-modifier-support-alt-approach?expand=1

@lb-
Copy link
Contributor Author

lb- commented Oct 17, 2023

Hey @marcoroth - hope it's ok to ping, just wondering if this could be reviewed. Thank you.

- Adds `mod` to the `defaultSchema` which will resolve to `Meta` on a MacOS like device and `Control` on Windows like
- Uses `mod` to be used as a key filter modifier for either `metaKey` or `ctrlKey` based on the resolved value
- Add unit tests and documentation for the `mod` key, and an example to the slideshow page
- Closes hotwired#654
@lb- lb- force-pushed the feature/654-mod-keyboard-modifier-support branch from 6b3291c to b59ab24 Compare February 7, 2024 23:02
@lb-
Copy link
Contributor Author

lb- commented Feb 7, 2024

Just rebased

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

Successfully merging this pull request may close these issues.

Feature Request - Cross-platform "modify" for keydown filter.
1 participant