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

Allow additional keyboard keys to create new tag when using taggable feature #956

Closed
peteyb opened this issue Oct 4, 2019 · 18 comments · Fixed by #971
Closed

Allow additional keyboard keys to create new tag when using taggable feature #956

peteyb opened this issue Oct 4, 2019 · 18 comments · Fixed by #971
Assignees

Comments

@peteyb
Copy link

peteyb commented Oct 4, 2019

Is your feature request related to a problem? Please describe.
We use the taggable feature to allow our users to enter email addresses to forward invitations to our site to their colleagues. Before moving to Vue.js and vue-select we used jQuery UI which allowed us to configure more than just the Enter key to create new tags.

Describe the solution you'd like
As email addresses do not contain spaces we would like our users to be able to hit the Space key to trigger new tags being created, not just the enter key.

Describe alternatives you've considered
I am reluctant to move back to jQuery within our vue spa

Additional context
Can you add a way of specifying a list of which keys trigger a new tag creation, defaulting to current Enter

@khamer
Copy link

khamer commented Oct 7, 2019

I created a PR for this functionality above. Let me know if this would provide the functionality you need.

@peteyb
Copy link
Author

peteyb commented Oct 11, 2019

@khamer I think that would work for our use case.

@Meekohi
Copy link

Meekohi commented Oct 15, 2019

Nice timing. I'm very interested in this.

sagalbot added a commit that referenced this issue Oct 25, 2019
@sagalbot
Copy link
Owner

sagalbot commented Oct 25, 2019

@khamer your PR is pretty close to how I imagined solving this problem. The challenge is that folks have different requirements for how the want the component to respond to input of all types, so I'd like to abstract it a bit higher.

#971 adds a new prop: map-keydown. This callback function receives an object mapping keyCodes to callback functions: { <keyCode>: <callback> }, as well as the component instance. The prop can be used to overwrite default functionality, or add handlers for different keys that the component doesn't normally listen for.

If I have a taggable input, and I want , or space to 'tag' the current option, and stay focused after entry, you could solve that with map-keydown.

<v-select taggable multiple no-drop :map-keydown="handlers"/>
export default {
  methods: {
    handlers (map, vm) {
      const createTag = e => {
        e.preventDefault();
        vm.typeAheadSelect();
        vm.searchEl.focus();
      };

      return {
        ...map, //  defaults
        32: createTag,  //  space
        188: createTag  //  comma
      };
    },
  },
};

This API does require a bit more knowledge of component inner-workings, but also provides a whole lot of flexibility for all types of use cases. What do you think of this solution?

@sagalbot sagalbot self-assigned this Oct 25, 2019
@Meekohi
Copy link

Meekohi commented Oct 26, 2019

I think you mean #971 ^

Personally I think it'd be great to merge #971 as the general purpose mechanism and then consider some "short hand" props for common use cases that just add the correct key-map under the hood.

@sagalbot
Copy link
Owner

thanks @Meekohi – updated to point to the right PR. I'll investigate some shorthand syntax today.

@khamer
Copy link

khamer commented Oct 27, 2019

I'm willing to rework my PR to make it more general. I'm not crazy about the syntax in #971 as it seems like the most common use case would be allowing multiple keys (say, space and enter) to both trigger the same action, and it doesn't seem consistent with vue to me.

I'd be more tempted to bubble up the events so that someone who wanted a more general implementation could write something like

<v-select multiple @keydown.space="(target, vm) => vm.typeAheadSelect()"/>
<v-select multiple v-on="bindings"/>

If this direction seems useful, I can make a separate PR for comparison. I still think it might be worth providing something like customSelectKeys or such as a shorthand for the most popular use case. With the above implementation, something like this would work, but I don't think this is obvious enough for the typical user:

<v-select multiple @keydown="({keyCode}, vm) => [32,188].includes(keyCode) && vm.typeAheadSelect()">

Let me know your thoughts.

@Meekohi
Copy link

Meekohi commented Oct 28, 2019

@keydown.space="(target, vm) => ... feels like a confusing syntax since it varies from what you'd expect: @keydown.space="event => ...

That is actually the first thing I attempted to solve this problem, and was surprised the events didn't bubble up. Is there someway to bubble up the expected event and still get access to the underlying vm to call things like vm.typeAheadSelect()?

@Meekohi
Copy link

Meekohi commented Oct 28, 2019

I do agree the #971 syntax could be pretty annoying if you want to do something after "any letter" or etc...

@khamer
Copy link

khamer commented Oct 28, 2019

@Meekohi I agree, I wrote the wrong thing - @keydown.space="(event, vm) => ... is what I meant; I think then you could either bind to just the event (@keydown.space="event => ...) or add a second parameter if you need vm. If you really only wanted to grab target, you could by doing @keydown.space="({target}) => ... for example.

Digging a little more, if we did this, it does seem like we could use custom keycodes like this:

Vue.config.keyCodes = { "space-or-enter": [32, 13] };
<v-select multiple @keydown.space-or-enter="(evt, vm) => vm.typeAheadSelect()"/>

Even supporting this syntax, I still think adding a convenience prop like selectOnCustom would be worth while for all the devs who don't need to something fancy.

@Meekohi
Copy link

Meekohi commented Nov 6, 2019

I guess the syntax issue can be solved in some tolerable ways if you generate the mapping object programmatically (using lodash here, but I assume this could be vanilla with some more work):

methods: {
  handlers (map, vm) {
    keyCodes = _.range(49,91) // any letter or number
    mappedCodes = _.mapValues(_.keyBy(keyCodes), customFunction => vm.typeAheadSelect)
    return {
      ...map, //  defaults
      ...mappedCodes, // custom
    };
  },
}
</script>

I'm still unclear on whether it's really possible or recommended to bubble up the event with the additional vm parameter like what is proposed above -- it seems like the concerns of the v-select component should somehow "stay inside the component"?

@Meekohi
Copy link

Meekohi commented Nov 6, 2019

So, I think I'm still in favor of #971 as the solution to this. It's significantly more generic, has a workable if imperfect syntax, and doesn't prevent adding in short-hand helpers for common use cases in a later PR just like in #958.

@sagalbot any additional help needed to get #971 in a mergeable state?

@sagalbot
Copy link
Owner

sagalbot commented Nov 7, 2019

@Meekohi I'm pretty torn about the API. It does feel a little too high level for the most common use cases. I think we need @khamer's solution embedded into the map-keydown prop. I'll take a crack at this in the next couple days.

@sagalbot
Copy link
Owner

sagalbot commented Nov 7, 2019

Okay - I think this is ready to go. Two new props:

  • map-keydown {Function}
  • selectOnKeyCodes {Array}

selectOnKeyCodes is an array of keyCodes that will trigger a typeAheadSelect. Any keyCodes in this array will prevent the default event action and trigger a typeahead select. By default, it's just [13] for return.

<!-- select on tab, space, return, or comma -->
<v-select :selectOnKeyCodes="[9, 32, 13, 188]" /> 

@Meekohi, @khamer what do you think?

@Meekohi
Copy link

Meekohi commented Nov 8, 2019

Looks great to me -- fun docs example with the @ => @gmail.com. Might want to leave a note in the docs somewhere that custom handlers will overwrite selectOnKeyCodes if there is a conflict.

sagalbot added a commit that referenced this issue Nov 8, 2019
@sagalbot
Copy link
Owner

sagalbot commented Nov 8, 2019

Good call @Meekohi, added that to the docs. Merged and I think I'll be able to get 3.3 out today. Thanks for all the feedback folks!

@Meekohi
Copy link

Meekohi commented Nov 8, 2019

Thanks @sagalbot!

@khamer
Copy link

khamer commented Nov 11, 2019

Yep, thanks @sagalbot! I might take a stab at an additional implementation that'd use $listeners - but none of that would get in the way of map-keydown or selectOnKeyCodes.

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

Successfully merging a pull request may close this issue.

4 participants