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

Add .bare event modifier, fix #5976 #5977

Merged
merged 5 commits into from
Oct 4, 2017
Merged

Conversation

nickmessing
Copy link
Member

@nickmessing nickmessing commented Jun 26, 2017

Add a new event modifier .bare to check if event is "bare" (no shift/ctrl/meta/alt key is
pressed)

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

Discussion: #5976

@nickmessing
Copy link
Member Author

If decided to be merged - should be documented

@@ -3660,7 +3660,8 @@ var modifierCode = {
meta: genGuard("!$event.metaKey"),
left: genGuard("'button' in $event && $event.button !== 0"),
middle: genGuard("'button' in $event && $event.button !== 1"),
right: genGuard("'button' in $event && $event.button !== 2")
right: genGuard("'button' in $event && $event.button !== 2"),
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove everything from packages? Those files are generated 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry, didn't know

Add a new event modifier `.plain` to check if event is "plain" (no shift/ctrl/meta/alt key is
pressed)

vuejs#5976
@nickmessing
Copy link
Member Author

@posva, removed the packages/* changes

@blake-newman
Copy link
Member

@nickmessing I added this PR should make it easier for new contributors. I'll see what the team thinks about this. #5984

@nickmessing
Copy link
Member Author

@blake-newman, I knew about dist, didn't know about packages :(

@blake-newman
Copy link
Member

@nickmessing yeah it's fairly easy to miss, and sometimes I add dist files by accident and it is annoying amending commits

@asumaran
Copy link

asumaran commented Jun 27, 2017

This doesn't seem to work if I do

<input @keyup.alt.plain="fire" >

Or is not the purpose of this PR to introduce this?

@posva
Copy link
Member

posva commented Jun 27, 2017

@asumaran plain will prevent the event if any modifier key is pressed, including alt. You can handle that your self easily in a custom function though 🙂

@asumaran
Copy link

@posva Thanks, I misunderstood the real issue 👍

@yyx990803
Copy link
Member

Is .plain the best word choice here? Somehow feels there could be something more elegant...

@javoski
Copy link
Member

javoski commented Jun 30, 2017

My English is not good🙂, and I can only think of pureunmixed or solo.

@posva
Copy link
Member

posva commented Jun 30, 2017

Even though I approved the change, @nickmessing made it clear that he isn't convinced either by the name either. We should think about it and we'll probably have a better name in a few days

on top of my head I can think about no-modifiers but modifier is also the name of that thing and after seeing many issues about misunderstanding the meaning of the modifier .prop (because it looks like props), I don't think using the word modifier is a good idea 😆 But I don't know any other worrd to define alt, ctrl & company

@hworld
Copy link

hworld commented Jun 30, 2017

"bare" maybe? A bare event seems to make sense meaning nothing additional

@nickmessing
Copy link
Member Author

@posva, actually, I really like .no-modifiers because that's what actually is (but we have vue's event modifiers too), maybe you could just do a poll with the members of core team to find out which one is reasonable?

@matthiasg
Copy link

It would be generally useful to have a .not modifier or defined prefixes to negate all existing modifiers.

Alternatively it would be ok if one could simple re-emit the event and define multiple handlers like such:

<div @contextmenu.ctrl="$emit($event)" @contextmenu.prevent.stop="$refs.menu.open($event)"></div>

This code actually works, but logs an error of course (because $event param should be a string but then it doesn't work).

If this worked the first @contextmenu.ctrl would cause the normal browser context menu when ctrl modifier is used. The second definition causes the other callback to e.g open a custom menu.

@posva
Copy link
Member

posva commented Jul 5, 2017

the not modifier is not possible at the moment. But adding too many modifiers make things super complicated. When you have 3 modifiers it's often time to do the filtering of keys in the bound function

@nickmessing
Copy link
Member Author

@posva, I actually like the idea of not modifier, it's going to be harder to implement but I love using :not in CSS, can we consider having .not(.ctrl.shift) as a syntax?

@matthiasg
Copy link

@posva what about a way to at least declare two handlers as in my example without causing exceptions in order to re-emit non-handled cases. It will be a question of specificity of course.

Or is there another way to cleanly re-emit bubbling native events (the fact that component events don't bubble is not ideal anyway, but that is another topic)

@posva
Copy link
Member

posva commented Jul 9, 2017

@nickmessing It could be added but I personally feel like it's too much because those modifiers generate js code atm, which makes things different from the order. IMO, it's better to write the actual javascript.

@matthiasg I'd add one single event handler and do the checks there

@matthiasg
Copy link

@posva sure we could do that, but in our architecture, we try to get most basic stuff done in the html or at the very least without specifics of structures like the html/dom eventing system. Vue and angular before are interesting to us, because they do not try to force people to use javascript for everything and our customers can follow at their own pace.

as for the specifics of ordering, a single prefixed not would be sufficient in my view, since it fills an obvious gap and there is no alternative to say do this when ctrl is pressed and do this when it doesn't match. If duplicate handlers were properly supported a not might not be necessary. But as it is now, I think it is syntactically incomplete due to implicitly ignoring all non defined modifiers.

@nickmessing
Copy link
Member Author

@matthiasg, .not in the way I described means a complete re-implementation of event modifiers. I kinda like the idea but I would vote no for it. .plain (or what name we're going to use) is enough for most use cases, if you need something more specific you better do that in methods.

@nickmessing
Copy link
Member Author

nickmessing commented Jul 26, 2017

/ping @posva, @kazupon, @blake-newman, @Kingwl

Any ideas what we do with this? Is .no-key-modifier the best name we end up with?

@posva
Copy link
Member

posva commented Jul 31, 2017

I think @hworld made a better proposition with bare
edit: We'll wait for @yyx990803 input on this. It's something that can wait anyway (because it can already be achieved)

@kyleshay
Copy link

kyleshay commented Aug 7, 2017

.default?

@Zexbe
Copy link

Zexbe commented Aug 10, 2017

@nickmessing How about a new event modifier .just to check if the event is "just" a specific key, or keys. It would make it an exact match.
@keyup.just.shift is just shift
@keyup.just.enter is just enter

@dsonet
Copy link
Contributor

dsonet commented Aug 13, 2017

@joshfisk Then I guess
@keyup.shift.only is more reasonable.

@Zexbe
Copy link

Zexbe commented Aug 13, 2017

@dsonet Yeah, ".only" works better. It is easy to understand, regardless of the order.
@keyup.only.shift
@keyup.shift.only

@nickmessing
Copy link
Member Author

@dsonet, @joshfisk, that would mean a pretty big change in the way modifiers are implemented, the easy way would be to add .only-shift and others but I don't think it's reasonable. I think you should open another issue to discuss that and to get team's feedback. I never need that one and I don't see real value in that but if you can provide good usecases maybe it can be considered.

@nickmessing nickmessing changed the title Add .plain event modifier, fix #5976 Add .bare event modifier, fix #5976 Aug 13, 2017
@Zexbe
Copy link

Zexbe commented Aug 13, 2017

@nickmessing It could initially support normal keys, but be expanded to support modifiers in the future. A common use is showing valid shortcut keys when you hold a modifier. This is commonly done with the Alt key, and menus.
".bare" would have the same issue if it was not supported.
@keyup.bare.shift

@nickmessing
Copy link
Member Author

@joshfisk, I will try to find an easy way of making .bare work with current modifiers so v-on:keyup.enter.bare.shift works like "if it's enter and shift is pressed and other modifiers are not pressed". Is that what you're looking for?

@Zexbe
Copy link

Zexbe commented Aug 14, 2017

@nickmessing
More like this:
"If enter and shift are pressed and no other key, or modifier on the keyboard is pressed".

Here are two example problems:

  1. Lets say you bind Y to Yes, and N to No, and have a span that holds the users choice called Answer.
    If the user pushes Y, and N at the same time you might want the answer to be Maybe, or for nothing to happen.

  2. You are making a help pop up page. "Ctrl H" brings up the help page, but advanced users can press "Ctrl H 3" to go directly to section 3 of the help page.

@nickmessing
Copy link
Member Author

@joshfisk, unfortunately that cannot be achieved since in scenario of both Y and N pressed two keyup event happend - one for Y and another one for N. I think this scenario is not common enough to be included in vue core but it's possible using custom directives with weird syntax like v-on-bare-key="shift+enter" v-on-bare-fn="myMethod" or with syntactic suger as a vue-loader preprocessor like @bareKeypress.shift.enter="myMethod". Anyway, that's another topic unrelated to this PR, can you contact me over official chat and we'll talk there about desired results and possible implementations?

@Zexbe
Copy link

Zexbe commented Aug 14, 2017

@nickmessing I see what you mean.

I prefer two of the suggested changes working together.
Adding a way to reverse all the modifiers (a modifier modifier), and adding the opposite of the ".bare" modifier.
Modifier - Meaning
".combo" - Shift, Alt, Ctrl, or Meta
Not sure what syntax would work for reversing a modifier
".!combo",".-combo",".~combo","noCombo" - Not (Shift, Alt, Ctrl, or Meta) //Would be equivalent to ".bare"

What is the reason 'stop', and 'prevent' are in the same lookup table as the modifier filters?
If they where in a separate table, the modifiers could just be "&&" together, and the "genGuard" could be removed. Reversing a modifier would then involve wrapping in parenthesis, and appending a "!" to the front when adding it to the code variable.

@nickmessing
Copy link
Member Author

@asumaran, @dsonet, @joshfisk, I managed to find a good way to make syntax like

<button @click.bare.shift="action">

work.

@kazupon, @posva, @Kingwl, @blake-newman, can you please take a look again?

@Zexbe
Copy link

Zexbe commented Aug 20, 2017

@nickmessing https://gist.github.com/JoshFisk/f407ac7c74c7123ae4594249cbcc3560
Adds .not, and .modified
.modified -> requires a modifier
.not -> reverses the condition of each modifier after it is specified
I like this approach, because it doubles the usefulness of the modifiers.
Note: It has a bug when you use not on prevent, or stop. It probably has other bugs. It is a proof of concept.

@nickmessing
Copy link
Member Author

@joshfisk, yeah, .not adds more flexibillity than my .bare solution but it adds a modifier for modifiers that looks too complex API in my opinion and not worth it. I don't think there are common enough usecases for it to be implemented.

@yyx990803 yyx990803 merged commit 9734e87 into vuejs:dev Oct 4, 2017
@nickmessing nickmessing deleted the feat-5976 branch October 4, 2017 20:25
ztlevi pushed a commit to ztlevi/vue that referenced this pull request Feb 14, 2018
Allow limiting the event to the exact system modifiers specified.
close vuejs#5976
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
Allow limiting the event to the exact system modifiers specified.
close vuejs#5976
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.