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

Save settings on enter keypress #8571

Merged
merged 1 commit into from
Oct 6, 2016

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Oct 6, 2016

Closes #8378. Save sense settings when pressing enter.

also removed some unused imports from Sense.

Remove unused imports
@kobelb
Copy link
Contributor

kobelb commented Oct 6, 2016

LGTM

@kobelb
Copy link
Contributor

kobelb commented Oct 6, 2016

Looks like it'd also be possible to use this directive, https://code.angularjs.org/1.4.7/docs/api/ng/directive/ngKeypress, not sure if we have any 'standard' way to do this. My LGTM stands though :)

if (event.which === 13) {
self.apply();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if there has been a style discussion or decision on arrow functions but if you use the arrow style I think you can get rid of the self variable (suggestion only and curious to hear if there is a reason not to use arrow style).

Copy link
Contributor Author

@thomasneirynck thomasneirynck Oct 6, 2016

Choose a reason for hiding this comment

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

I wanted a named reference to the function so I could unbind it. If I'd used arrow syntax, I would have to assign it to a variable, which to me defeats the point of using an anonymous function. Happy to change it though, no big preference either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, thanks for the explanation. I thought the main benefit of arrow functions was to avoid the binding or 'this' assignments, I didn't realize that people use it only for anonymous function situations. I took a look at the airbnb style guide (in case we make a decision to use that) and it looks like it suggests arrow fn or .bind, but favors those both over saving a 'this' reference. https://github.com/airbnb/javascript#naming--self-this

But since we haven't yet committed to using that style guide, so I'm good with it as is. :)

@thomasneirynck
Copy link
Contributor Author

@kobelb I just could not get that ng-keypress directive to fire at all inside the settings directive. Maybe I overlooked something simple..

@thomasneirynck thomasneirynck merged commit 9fe13ed into elastic:master Oct 6, 2016
elastic-jasper added a commit that referenced this pull request Oct 6, 2016
---------

**Commit 1:**
Save settings on enter keypress

Remove unused imports

* Original sha: 09d3346
* Authored by Thomas Neirynck <[email protected]> on 2016-10-06T15:46:48Z
elastic-jasper added a commit that referenced this pull request Oct 6, 2016
---------

**Commit 1:**
Save settings on enter keypress

Remove unused imports

* Original sha: 09d3346
* Authored by Thomas Neirynck <[email protected]> on 2016-10-06T15:46:48Z
thomasneirynck added a commit that referenced this pull request Oct 6, 2016
thomasneirynck added a commit that referenced this pull request Oct 6, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
---------

**Commit 1:**
Save settings on enter keypress

Remove unused imports

* Original sha: afd4b4171b0298311f3d3d2726604fefc259b4b4 [formerly 09d3346]
* Authored by Thomas Neirynck <[email protected]> on 2016-10-06T15:46:48Z


Former-commit-id: de05644
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 21, 2024
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 22, 2024
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 22, 2024
kibanamachine added a commit to alexwizp/kibana that referenced this pull request Feb 23, 2024
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 26, 2024
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.

4 participants