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: js2r-universal-expand-contract func to work with array,object,func #79

Merged
merged 1 commit into from Oct 25, 2016
Merged

Conversation

futurist
Copy link
Contributor

I made this PR since I found it's very useful in my daily usage.

It's just expand and contract closest expression, for array, object, function, function-call

Please review, thanks!

@NicolasPetton
Copy link
Collaborator

Thanks for the PR!

I'm not sure about adding another way to expand/contract, while keeping the current ones.
I would rather either 1. stay with what we have or 2. change to what you are suggesting, but then we should remove the current functions.

@futurist
Copy link
Contributor Author

In my daily use I've seldom use the c a, e a or c o, e o, instead, using this universal function in most of the case.

In fact, by moving point in the right place, I've use this function for all cases.

@NicolasPetton
Copy link
Collaborator

In fact, by moving point in the right place, I've use this function
for all cases.

Before we replace the previous functions with yours, I'd like to find
better names for them, and better keybindings (what about js2r-expand-node-at-point' andjs2r-contract-node-at-point'?).

For consistency, a 2 character keybinding would be great.
I'd also split the function in 2, one for contracting and another one
for expanding, and remove the universal argument.

@futurist
Copy link
Contributor Author

futurist commented Oct 19, 2016

Please review the commit, please suggesting the key binding. (will fix CI after decide the right key)
below is my suggestions:

  1. ee and cc (easy to remember and press)
  2. ee and ec (e==exp, and press ee is very quick)
  3. ne and nc (n==node)
  4. be and bc (b==bracket)

@NicolasPetton
Copy link
Collaborator

It's nice, thanks! The tests are failing though, could you have a look?

Also, before merging, could you remove the previous functions and keybindings completely, and update the readme?

@futurist
Copy link
Contributor Author

I think the new key-bindings should be carefully choosing, now it's ee and cc,

below is my suggestions:

  1. ee and cc (easy to remember and press)
  2. ee and ec (e==exp, and press ee is very quick)
  3. ne and nc (n==node)
  4. be and bc (b==bracket)

What do you think?

@futurist
Copy link
Contributor Author

Oh, and the version number, I've bumpped a patch version.

But by removing multiple key-bindings, should it be more advanced?

@NicolasPetton
Copy link
Collaborator

James Yang notifications@github.com writes:

But by removing multiple key-bindings, should it be more advanced?

Yes, it should be a major version bump.

@futurist
Copy link
Contributor Author

Bumpped version into 0.8.0, add docs about the change log.

Also please allow added me to the contributors 😄

@NicolasPetton
Copy link
Collaborator

@futurist looks good, thanks! Just squash your commits and we're good to go!

@futurist
Copy link
Contributor Author

please review.

@NicolasPetton
Copy link
Collaborator

Excellent! 👍 thank you!

@NicolasPetton NicolasPetton merged commit bd73f03 into js-emacs:master Oct 25, 2016
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.

None yet

2 participants