[WIP] Implemented `keybind` module to handle keyboard shortcuts #186

lamemakes · 2024-03-22T04:28:57Z

Closes #146. The goal here was to create a simple system that could be reused to handle keyboard shortcuts. Thus as
Stract grows and potentially implements more features/views, new shortcuts can be easily added. Most of the TSDoc comments speak for themselves but in summary:

Created new class Keybind to handle all needed Keybindings for a route
- For simplicity, limited to a single "normal" key that can be combined with shift, alt, & ctrl
- Class obj is initialized using params for the keys for the shortcut, and their callback
Added search binding callbacks in keybind.ts
- Done here to prevent bloat in /search/+page.svelte
- exported in a single var to prevent unwieldy imports in /search/+page.svelte
Mapped search keyboard shortcuts/functionality according to DuckDuckGo's
Added new preference option to allow the user to globally disable keyboard shortcuts

Let me know whatever feedback you've got, totally willing to refactor to your preferences. Cheers!

lamemakes · 2024-03-22T04:34:51Z

Drafting as my main branch was out of sync so this needs merge help but will still take feedback in the meantime @mikkeldenker!

mikkeldenker

The functionality seems to be absolutely spot-on, but we need to make some changes for maintainability. In general, we should avoid using query selectors and instead give references to binded elements to the relevant functions. This way, it's very easy to see directly in the code which components depend on eachother and avoid them coming out of sync.

I think you can bind multiple results in search/+page.svelte like this

let resultElems: Result[] = [];
...
{#each results.webpages as webpage, resultIndex (`${query}-${webpage.url}`)}
    <div animate:flip={{ duration: 150 }}>
        <Result
        bind:this={resultElems[resultIndex]}
        {webpage}
        {resultIndex}
        on:modal={openSearchModal(webpage)}
        />
    </div>
{/each}

I know there is a ton of information here, but please feel free to ask if anything is unclear.

frontend/src/lib/keybind.ts

mikkeldenker · 2024-03-23T15:05:43Z

frontend/src/lib/keybind.ts

+  private keyEnumFromString(str: string): Keys | undefined {
+    for (const [_, key] of Object.entries(Keys)) {
+      if (key == str) return key;
+    }
+
+    return undefined;
+  }


When Key is a type, this can be written as

private keyFromString(str: string): Key | undefined { return Keys.find((key) => key === str); }

frontend/src/routes/search/+page.svelte

mikkeldenker · 2024-03-23T15:11:04Z

frontend/src/lib/keybind.ts

+/**
+ * Used to indicate what direction {@link navigateResults} should go (up/down)
+ */
+type Direction = 1 | -1;


type Direction = "up" | "down";

is a bit more descriptive. The type is also pretty self-explanatory and the TsDoc string therefore only clutters it a bit and could be removed

frontend/src/lib/keybind.ts

mikkeldenker · 2024-03-23T15:30:29Z

frontend/src/lib/keybind.ts

+const openResult = (e: KeyboardEvent) => {
+  if ((e.target as HTMLElement).className.includes('result-main-link')) {
+    const resultLink = e.target as HTMLAnchorElement;
+    window.location.href = resultLink.href;
+  } else {
+    const results = document.getElementsByClassName('result-main-link');
+    if (results.length > 0) {
+      window.location.href = (results[0] as HTMLAnchorElement).href;
+    }
+  }
+};


could be written as

const openResult = (_e: KeyboardEvent, refs: Refs) => { const focusedResult: Result | undefined = refs.results.find((result) => result.hasFocus()); if (focusedResult) { focusedResult.open(); } };

with related .open() and .hasFocus() functions in Result.svelte

mikkeldenker · 2024-03-23T15:31:51Z

frontend/src/lib/keybind.ts

+const openResultInNewTab = (e: KeyboardEvent) => {
+  // Ensure target event is a result url
+  if ((e.target as HTMLElement).className.includes('result-main-link')) {
+    const resultLink = e.target as HTMLAnchorElement;
+    window.open(resultLink.href, '_blank', 'noopener');
+  }
+};


could be written as

const openResultInNewTab = (_e: KeyboardEvent, refs: Refs) => { const focusedResult: Result | undefined = refs.results.find((result) => result.hasFocus()); if (focusedResult) { focusedResult.openInNewTab(); } }

with related .openResultInNewTab() and .hasFocus() functions in Result.svelte

mikkeldenker · 2024-03-23T15:33:35Z

frontend/src/lib/keybind.ts

+  const msLink = document.getElementById('misspell-link');
+  if (msLink) window.location.href = (msLink as HTMLAnchorElement).href;


Should use Refs with a binding for the mispell link added to the type instead of .getElementById and could then also use a .open() method

mikkeldenker · 2024-03-23T15:38:37Z

frontend/src/lib/keybind.ts

+  // Ensure target event is a result url
+  if ((e.target as HTMLElement).className.includes('result-main-link')) {
+    const searchBar = document.getElementById('searchbar') as HTMLInputElement;
+    if (searchBar) {
+      // Parse the result url, pull out the hostname then resubmit search
+      const resultLink = new URL((e.target as HTMLAnchorElement)?.href);
+      searchBar.value += ` site:${resultLink.hostname}`;
+      searchBar.form?.submit();
+    }
+  }


should not directly reference the dom elements but instead call related .userQuery() and search(`${query} site:${resultLink.hostname}`) methods. ResultLink.svelte should also have a .url() method

can you expand on .userQuery() & search(...)? I couldn't find any userQuery methods anywhere and was just going to pull it from PageData and pass it in a wrapper. As for search I was going to use this if that's what you were referring to. Wasn't sure if these are existing need to be built out.

Yea I think it would be easiest to create those as new methods in Searchbar.svelte

export const userQuery = () => lastRealQuery; export const search = (q: string) => { if (formElem && inputElem) { inputElem.value = q; formElem.submit(); } };

with an added formElem bind

let formElem: HTMLFormElement | undefined; ... <form action="/search" class="flex w-full justify-center" id="searchbar-form" bind:this={formElem} method={$postSearchStore ? 'POST' : 'GET'} >

The search function in search.ts effectively just calls the api and returns the results. Calling the search function in Searchbar.svelte would ensure that the user interface gets updated correctly with the new results.

domainSearch can then be something like

const domainSearch = (_e: KeyboardEvent, refs: Refs) => { const focusedResult: Result | undefined = refs.results.find((result) => result.hasFocus()); if (refs.searchbar && focusedResult) { const query: string = refs.searchbar.userQuery(); const url: string = focusedResult.getMainLink().url(); const domain = new URL(url).hostname; const domainQuery = `site:${domain}`; if (query.includes(domainQuery)) { refs.searchbar.search(query); } else { refs.searchbar.search(`${query} site:${domain}`); } } }

Where getMainLink would be a new method in Result.svelte

export const getMainLink = () => mainLink

and url would be a new method in ResultLink.svelte

export const url = () => href;

mikkeldenker · 2024-03-23T15:40:07Z

frontend/src/routes/search/+page.svelte

+    if (!((event.target as HTMLElement).nodeName === 'INPUT')) {
+      keybind.onKeyDown(event, $useKeyboardShortcuts);
+    }


this could be

if (event.target != searchbar?.getInputElem()) { keybind.onKeyDown(event, $useKeyboardShortcuts); }

lamemakes · 2024-03-23T21:03:30Z

This feedback is phenomenal! I really appreciate you taking the time to give in depth feedback. Svelte is completely new to me as I'm coming from an Angular/Vue/Django background so these pointers have been incredibly helpful. Especially cool all of the things you can do with bindings in regards to HTML Elements. I had no idea and wince a bit whenever I had to do DOM manipulation. I'll have some revisions up shortly!

lamemakes · 2024-03-27T03:49:04Z

Changes will be up tomorrow - didn't catch the keybindings in Searchbar so those will also be included

lamemakes · 2024-03-29T03:28:43Z

frontend/src/lib/keybind.ts

-  callback: (e: KeyboardEvent) => void;
+  key: Key;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  callback: (e: KeyboardEvent, context: any) => void;


had a bit of a battle with typing here. I tried to create a generic and use it both here (ie. context: Context) and then again on line 39 with export class Keybind<Context> but everything I tried complained or didn't register that they would be the same type when implemented. this has worked fine but totally open to any suggestions

lamemakes · 2024-03-29T03:31:23Z

frontend/src/lib/keybind.ts

-/**
- * Redirect to the `Did you mean:` link (if exists)
- */
-const goToMisspellLink = () => {


This is also the only one that I can't get working. I'm pretty sure if has to do with the logic of the spell correction and it's conditional rendering - let me know if you look over this and see anything right off the bat

lamemakes · 2024-03-29T03:36:32Z

Was hoping this method of implementation didn't clutter things but the aim was re-usability as Stract grows. Implementing it in the search bar felt like some overhead was definitely eliminated, along with code not being duplicated which was nice. Once feedback is given on those two comments I'll move the PR out of draft. Cheers!

It's always used with 'Refs' as context, so there is no need to have it generic

The functionality didn't work (for instance enter didn't trigger a search). It would require a lot of aditional complexity in 'Keybind' to also support the use case from searchbar. It's okay to have some code duplication if this results in a simpler solution that will therefore be more readable and mantainable long term

This forces us to not rely on direct manipulation of the event, but instead implement the necesarry functionality in helper methods in the different components

mikkeldenker

Neat! I hope it's okay I went ahead and finished the last few things so we could get this merged. I tried to document it a bit in the commit messages, but in broad strokes:

The callbacks in Keybind always use Refs so there is no need to make it generic.
I think it's better to not use Keybind in the searchbar and have a simple keydown match instead. It would add a fair bit of complexity to Keybind to be able to use it both in the shortcuts and searchbar, and I think this added complexity wouldn't be worth it compared to the (very small) code replication.
The callbacks can all be implemented without having to know about the keyboard event that triggered them.

Great work!

lamemakes · 2024-04-04T23:49:47Z

This is great, thanks a ton for the fixes! Are you sure the keybind class isn't adding bloat? I can totally do a quick refactor and toss it all in the +page.svelte like the search's bindings. Cheers!

mikkeldenker · 2024-04-05T07:38:07Z

I actually quite like to have it in a separate file to avoid routes/search/+page.svelte becoming too enormous. I guess we can move keybind.ts into routes/search to indicate this is the only place we currently use it though.

Implemented keybind module to handle keyboard shortcuts

e57f090

lamemakes marked this pull request as draft March 22, 2024 04:33

lamemakes changed the title ~~Implemented keybind module to handle keyboard shortcuts~~ [WIP] Implemented keybind module to handle keyboard shortcuts Mar 22, 2024

mikkeldenker requested changes Mar 23, 2024

View reviewed changes

Merged with main

25c8a19

Removal of direct DOM querying and the addition of searchbar keybindings

dae5d01

lamemakes commented Mar 29, 2024

View reviewed changes

mikkeldenker added 3 commits April 2, 2024 09:30

Remove generics from 'keybind'

e83a78a

It's always used with 'Refs' as context, so there is no need to have it generic

Remove need to know about keyboard event in keybind callbacks

2b0f882

This forces us to not rely on direct manipulation of the event, but instead implement the necesarry functionality in helper methods in the different components

mikkeldenker marked this pull request as ready for review April 2, 2024 11:14

forgot to remove a console.log...

ccc5abd

mikkeldenker approved these changes Apr 2, 2024

View reviewed changes

mikkeldenker merged commit cc612c5 into StractOrg:main Apr 2, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implemented `keybind` module to handle keyboard shortcuts #186

[WIP] Implemented `keybind` module to handle keyboard shortcuts #186

lamemakes commented Mar 22, 2024

lamemakes commented Mar 22, 2024

mikkeldenker left a comment

mikkeldenker Mar 23, 2024 •

edited

Loading

mikkeldenker Mar 23, 2024

mikkeldenker Mar 23, 2024

mikkeldenker Mar 23, 2024

mikkeldenker Mar 23, 2024

mikkeldenker Mar 23, 2024

lamemakes Mar 27, 2024

mikkeldenker Mar 27, 2024

mikkeldenker Mar 23, 2024 •

edited

Loading

lamemakes commented Mar 23, 2024

lamemakes commented Mar 27, 2024

lamemakes Mar 29, 2024 •

edited

Loading

lamemakes Mar 29, 2024

lamemakes commented Mar 29, 2024

mikkeldenker left a comment

lamemakes commented Apr 4, 2024

mikkeldenker commented Apr 5, 2024

		const msLink = document.getElementById('misspell-link');
		if (msLink) window.location.href = (msLink as HTMLAnchorElement).href;

[WIP] Implemented keybind module to handle keyboard shortcuts #186

[WIP] Implemented keybind module to handle keyboard shortcuts #186

Conversation

lamemakes commented Mar 22, 2024

lamemakes commented Mar 22, 2024

mikkeldenker left a comment

Choose a reason for hiding this comment

mikkeldenker Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkeldenker Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

lamemakes commented Mar 23, 2024

lamemakes commented Mar 27, 2024

lamemakes Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lamemakes commented Mar 29, 2024

mikkeldenker left a comment

Choose a reason for hiding this comment

lamemakes commented Apr 4, 2024

mikkeldenker commented Apr 5, 2024

[WIP] Implemented `keybind` module to handle keyboard shortcuts #186

[WIP] Implemented `keybind` module to handle keyboard shortcuts #186

mikkeldenker Mar 23, 2024 •

edited

Loading

mikkeldenker Mar 23, 2024 •

edited

Loading

lamemakes Mar 29, 2024 •

edited

Loading