Hotkey #2149
Hotkey #2149
Conversation
|
@elijah-potter hey! I was gone for a moment but now I'm back trying to get this feature in Harper again. I was previously getting the lint-framework errors where it was failing to build on local and in Actions, so I remade the branch locally to ensure that everything builds fine, which it does, but it seems that it is once again having issues building lint-framework in actions. Do you know what the issue may be? My code is up to date with Harper's master. Thank you! |
|
I'm so sorry it took me so long to take a look at this. It looks like, in |
elijah-potter
left a comment
elijah-potter
left a comment
There was a problem hiding this comment.
This looks and feels great. There are a few bits I'd like to have addressed before we merge this (mostly just fit and finish items). This is stellar work and I'm really excited to start using this in my day-to-day.
Thank you!
| <div class="flex items-center justify-between"> | ||
| <div class="flex flex-col"> | ||
| <span class="font-medium">Apply Last Suggestion Hotkey</span> | ||
| <span class="font-light">Hotkey to apply the most likely suggestion to previously incorrect lint.</span> |
There was a problem hiding this comment.
I think this description could be reworded.
There was a problem hiding this comment.
Thanks for the review! I changed it to "Applies suggestion to last highlighted word.", does that work? I thought that "last highlighted word" might make more sense to a user than "previously incorrect lint"
| <div class="space-y-5"> | ||
| <div class="flex items-center justify-between"> | ||
| <div class="flex flex-col"> | ||
| <span class="font-medium">Apply Last Suggestion Hotkey</span> |
There was a problem hiding this comment.
This title's style does not match the others. See the "Activation Key"'s heading.
| <div class="space-y-5"> | ||
| <div class="flex items-center justify-between"> | ||
| <div class="flex flex-col"> | ||
| <span class="font-medium">User Dictionary</span> |
There was a problem hiding this comment.
It appears that these titles may have been accidentally duplicated? There are two "User Dictionary" headings.
There was a problem hiding this comment.
whoops, fixed!
| <span class="font-light">Hotkey to apply the most likely suggestion to previously incorrect lint.</span> | ||
| </div> | ||
| <Textarea readonly bind:value={buttonText} /> | ||
| <Button size="sm" color="light" style="background-color: {isBlue ? 'blue' : ''}" bind:this={modifyHotkeyButton} on:click={() => {startHotkeyCapture(modifyHotkeyButton); isBlue = !isBlue}}>Modify Hotkey</Button> |
There was a problem hiding this comment.
I think it would be more intuitive if the text itself changed while the hotkey capture was listening. Maybe have it say something like, "Press your desired key combination now". Or, something shorter. Either way, as it is right now, it isn't super obvious that when it's listening.
There was a problem hiding this comment.
Thanks, it should now say "Press desired hotkey combination now." after clicking the modify button
| let activationKey: ActivationKey = $state(ActivationKey.Off); | ||
| let userDict = $state(''); | ||
| let modifyHotkeyButton: Button; | ||
| let hotkey: Hotkey = $state({ modifiers: ['Ctrl'], key: 'e' }); |
There was a problem hiding this comment.
I really like this as a default key combination. It just feels... right.
| if (match) { | ||
| event.preventDefault(); | ||
| event.stopImmediatePropagation(); | ||
| const previousBox = this.lastBoxes[this.lastBoxes.length - 1]; |
There was a problem hiding this comment.
Because of how the lastBoxes are pulled together, you can't make any guarantees on their ordering. As a result, this strategy of just applying the last one results in somewhat random behavior.
You may want to instead iterate through them and use the functions in Box.ts to find the nearest one to the cursor. We do this already when selecting a box to open when the "Activation Key" is triggered.
Take a look at the PopupHandler.openClosestToCaret function.
There was a problem hiding this comment.
thanks, I took a look and used the methods from Box to get the closest box to the caret. How does it look now? I totally agree with your assessment here and I think it should behave more inline with what would be expected now.
2 participants
Issues
closes #1904
I am remaking this PR and the SpellChecking PR as I was having too many problems building lint-framework that should be resolved by just making new PRs.
Description
This change adds a hotkey that corrects the last incorrect word as well as an option to change the hotkey in the settings menu.
To change the hotkey, click the "Modify Hotkey" button which will then listen for your hotkey change and display it in the textarea next to the button. To be abundantly clear, this is a WIP PR so far and should not be merged as there are still some issues to iron out.
Demo
Screenshot of the new option in the settings menu:
How Has This Been Tested?
Tested in blank.page where the hotkey is confirmed to correct the last incorrect word, as well as the Github editor, reddit text editor, and in Gmail. Confirmed to be working in all of these.
Checklist