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

Feature/multiline support #37

Closed
wants to merge 12 commits into from
Closed

Feature/multiline support #37

wants to merge 12 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 3, 2021

Multiline support complete

@jeremychone
Copy link
Contributor

wow, cool, give me few days to look at it, to make sure it does not change the nature of the plugin (meaning, don't change the content inside selection). Other than that, the summary above looks good.

@ghost
Copy link
Author

ghost commented May 3, 2021

I hadn't thought about apostrophes in my original PR. I added somewhat of a naïve solution to ignore them, but it should handle most use cases.

@ghost
Copy link
Author

ghost commented May 5, 2021

The code could be prettier, but it is working! I tested all the cases I could think of and it seems pretty responsive. I'm calling it good. Let me know if you find anything that needs a patch.

@jeremychone
Copy link
Contributor

@jameygleason First, thanks for the hard work, I think it will be awesome. Just want to make sure, when we select some text it will change the outside, right? This was a feature of this plugin that I think user appreciated, so, multiline is great if it does not break this. Make sense?

@ghost
Copy link
Author

ghost commented May 6, 2021

I did a video manually running through all of the test cases. I think it covers current functionality. Look forward to hearing your thoughts.

https://www.youtube.com/watch?v=m_uc61JiLiQ

@jeremychone
Copy link
Contributor

Watched the video. Wow, this is very good work. Give me end of weekend, I will definitely make the final check, but this is impressive. Users should be happy!

@jeremychone
Copy link
Contributor

jeremychone commented May 10, 2021

@jameygleason Ok, I accepted the two first one, I tested the multi-line one, and it looks awesome, but it does change a little behavior that I would like to keep (selection content should be ignored from any logic).

See my quick video here: https://www.youtube.com/watch?v=X9-BM1DNrHQ

If that can be fixed, we should be good to go.

Feel free to add the commit to this branch, I have it as an orgin/branch, so, I will pull it when ready.

@jeremychone
Copy link
Contributor

@jameygleason So, i was thinking, perhaps what would make sense is put this multiline/traditional in another hot key, like cmd shift ' , to shift toggle quotes to be smarter. This way, we have the dumb way as today, and the smarter way with this multiline support and escaped skip.

What do you think?

@ghost
Copy link
Author

ghost commented May 18, 2021

@jeremychone sorry for the delayed response. I agree with you, the ignore selection feature is a good one. Also the multiline feature is only basically useful for backticks*, but I'm interested in finding a solution without adding another key command if at all possible. Maybe it could detect multiline and only do backticks, but also ignore selected areas?

(*Open to feedback for use cases of multiline with single or double strings?)

I've considered adding the same functionality for escaping quotes. Maybe with the [ctrl / cmd] + \ <- this feature seems like it would be best served as a single line feature since escaped quotes are unnecessary in backticks.

Share your thoughts.

@jeremychone
Copy link
Contributor

@jameygleason sorry for the delay as well. Need to think more about it and come back. But in short, here are current thinking:

  1. I really like the work in this pull request, so, I would like to integrate it in a way.
  2. The main conflict I see what the existing behavior is not the multi line, but more the escaped quotes recognition, which is great but change the behavior. Right now, I change toggle the escaped quotes and then, change the outer one by selecting the
  3. So, this is what I am thinking that a shift modifier might be good.
  4. Also, looking at the latest bug, Support multi character beginning/end strings, e.g. Ruby's Percent Strings, here docs etc. #38, that reminds me that one cool feature added by a pull request and we need to make sure to not break it. (I should fix it for now).

Anway, let's think more about it. If you could refactor the code to not replace add, so that it just becomes a choice of mapping (or not) the keys, that would be great for now.

@ghost
Copy link
Author

ghost commented May 24, 2021

All sounds good. I'm hoping to finish up a project this week so I can have time for these changes.

@jeremychone
Copy link
Contributor

Closing this merge for now, because of issues noted in comment above

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.

2 participants