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

able to exit from palette using 1 or more backspaces on empty search #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rouilj
Copy link
Contributor

@rouilj rouilj commented Feb 9, 2023

On mobile, there is no good way to exit from the palette using the keyboard. Most mobile keyboards don't have ESC keys, or they are on a secondary keyboard layout. The backspace key however is usually available on the primary keyboard layout.

This patch adds the ability to configure the number of consecutive backspaces to exit the palette. The backspaces only count if the search input is empty. If the number of required backspaces is > 1, the number of remaining backspaces is shown if it is less than 5. This feedback is generated by a template string. i18n may be an issue. YAGNI says not to fix this now 8-) .

Docs updated no examples changed. Works in Firefox and Chrome mobile and desktop.

NOTE: this also patches the onKeyDown in SearchField to use e.key rather than e.code otherwise it doesn't work on mobile where it is most useful.

On mobile, there is no good way to exit from the pallete using the
keyboard. Most mobile keyboards don't have ESC keys, or they are on a
secondary keyboard layout. The backspace key however is usually availble
on the primary keyboard layout.

This patch adds the ability to configure the number of consecutive
backspaces to exit the palette. The backspaces only count if the
search input is empty. If the number of required backspaces is > 1,
the number of remaining backspaces is shown if it is less than 5.
This feedback is generated by a template string. i18n may be an
issue. YAGNI says not to fix this now 8-) (if I even has a clue as to
how.)

Docs updated no examples changed. Works in Firefox and Chrome mobile
and desktop.

NOTE: this also patches the onKeyDown in SearchField to use e.key
rather than e.code otherwise it doesn't work on mobile where it is
most useful.
@rouilj
Copy link
Contributor Author

rouilj commented Feb 11, 2023

If you want to try it, go to: https:// rouilj.dynamic-dns.net/demo (remove the space) and try typing he then backspace over it, emptying the search input, and then one more time and follow directions 8-).

Copy link
Owner

@benwinding benwinding left a comment

Choose a reason for hiding this comment

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

I think there could be a new component SearchFeedback or something, instead of manipulating the DOM within svelte.

If you want to try it, go to: https:// rouilj.dynamic-dns.net/demo (remove the space) and try typing he then backspace over it, emptying the search input, and then one more time and follow directions 8-).

Nice work! That's a nice tool to use 👍

Comment on lines +33 to +45
function setElFeedback(message) {
if (! inputElFeedback) {
inputElFeedback = document.getElementById(
inputName + "-feedback")
}

inputElFeedback.innerText = message
if (!!message) {
inputElFeedback.classList.add('open');
} else {
inputElFeedback.classList.remove('open');
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

No need to imperatively set the DOM with svelte, you can just make a new component in the directory and "pass it properties" or "hide it" etc... just like we do with SearchField.svelte. Maybe SearchFeedback.svelte or something would be a good name 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea how to dynamically add a component at run time. From what I see the component
structure is hard coded at instantiation time and props don't get modified at run time without
something like #39.

} else if (keyCode === "escape") {
onBlur();
} else {
currentBackspaceCount = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

I think we might want a tiny function for this:

function resetBackspaceCount() {
  currentBackspaceCount = 0;
}

Then call it everywhere.

Also looks like we want to reset in basically all cases except when keyCode === 'backspace', so maybe this logic can just be moved below:

if (keyCode === "backspace" && backspaceCloseCount) {
  if (!inputValue){
    currentBackspaceCount++
    if (currentBackspaceCount >= backspaceCloseCount) {
      onBlur();
    } else {
      // notify the user
      let left = backspaceCloseCount - currentBackspaceCount
      if (left < 5) {
        setElFeedback(`${left} more to exit`)
      }
    }
  }
} else {
  resetBackspaceCount();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also the patch to allow shift+backspace to erase the input. That does trigger setting the count to 0.

Also handling shift+backspace has to come before this code in the if cascade.
Otherwise the backspace gets eaten by this code. Hmm, I wonder if I need an additional condition there that makes sure it's a pure backspace without any modifiers (ctrl, space, alt, meta etc.).

src/SearchField.svelte Outdated Show resolved Hide resolved
Co-authored-by: Ben Winding <benw@canva.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants