Nothing Special   »   [go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

add keyboard access to search and copy/paste behaviour #46

Merged
merged 6 commits into from
Sep 4, 2023
Merged

add keyboard access to search and copy/paste behaviour #46

merged 6 commits into from
Sep 4, 2023

Conversation

the6p4c
Copy link
Contributor
@the6p4c the6p4c commented Sep 3, 2023 •
edited
Loading

🐶

  • adds keyboard access to search through '/' key
    • search can also be dismissed with escape
  • copy copies the current point/points to the clipboard (if no other text on the page is selected)
  • paste either:
    • jumps to the point/points from the clipboard, if the clipboard contains only one grapheme cluster
    • otherwise, performs a search for the full clipboard contents

this fixes #21 and #14 :3

@the6p4c
Copy link
Contributor Author
the6p4c commented Sep 3, 2023 •
edited
Loading

i could actually balloon this pr and address #14, cause that sounds fun. though i think i'd go with

  • paste of one character jumps to that character
  • paste of multiple characters searches for the string

@the6p4c the6p4c force-pushed the add-search-shortcut branch 2 times, most recently from db1703a to a137c0b Compare September 3, 2023 07:02
@the6p4c the6p4c changed the title add keyboard shortcut for search ('/' key) add keyboard access to search and copy/paste behaviour Sep 3, 2023
@the6p4c
move preventDefault calls as early as possible
6687829
@the6p4c
bail out of effect if search is open
fd8c425 
this is in contrast to an unnecessary check when adding the event
listeners.
delan
delan reviewed Sep 4, 2023
View reviewed changes
Copy link
Owner
@delan delan left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great with a couple of minor issues :3

Note that there are some TypeScript errors because the DOM typings are out of date. We can fix that by upgrading TypeScript, so I’ll take care of that before merging.

src/new.tsx Show resolved Hide resolved
src/new.tsx Outdated Show resolved Hide resolved
src/new.tsx Outdated Show resolved Hide resolved
src/new.tsx Outdated Show resolved Hide resolved
@delan delan merged commit 446d395 into delan:master Sep 4, 2023
@ar1a
Copy link
ar1a commented Sep 4, 2023

thanks for solving my issue :)

@the6p4c
Copy link
Contributor Author
the6p4c commented Sep 6, 2023

thanks for solving my issue :)

poggers (am i using this right.)

@ar1a
Copy link
ar1a commented Sep 7, 2023

yes

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

@delan delan delan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

escape key to cancel search
3 participants
@the6p4c @ar1a @delan