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

autocomplete tags/components #247

Merged
merged 8 commits into from
May 20, 2024
Merged

autocomplete tags/components #247

merged 8 commits into from
May 20, 2024

Conversation

knotbin
Copy link
Contributor

@knotbin knotbin commented May 19, 2024

Description

Tags in HTML, JS, TS, JSX, and TSX are now autocompleted. When you type <div> for example, the closing tag </div> will be autocompleted. If you press enter, you will be put on a new line in between the opening and closing and that new line will be indented. All tag attributes are ignored in the closing tag.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

Screen.20Recording.202024-05-19.20at.206.mp4

@austincondiff
Copy link
Collaborator

austincondiff commented May 20, 2024

This looks really good! To be consistent with other autocompleted brackets though, the completion should occur when you type the next character after >. For example…

  1. <|
  2. <d|
  3. <di|
  4. <div|
  5. <div>|
  6. One of the following happens
  • 6a. <div>X|</div> (autocomplete occurred after typing X)
  • 6b. If pressing return, the following happens…
<div>
    |
</div>

Autocomplete occurred after pressing return key. New lines were also created and the cursor is indented.

There might later be a setting in “Text Editing” to determine when the autocomplete happens not just for these tags but also for brackets, braces, and parenthesis.

Auto-close braces: Immediately | After next character

@knotbin
Copy link
Contributor Author

knotbin commented May 20, 2024

While I think this is appropriate for other autocompleted brackets because you always intend to put something in the middle of them, there are many times when you put in a tag such as a React component you already made and want to put nothing in the middle. Because of this, I believe it's best to follow the way that most editors complete these tags, which is right after you type them, as you outlined in the issue discussion.

@austincondiff
Copy link
Collaborator

austincondiff commented May 20, 2024

When I said that in the issue discussion, I forgot about how Xcode autocompletes brackets, my mistake. We should probably be consistent with how Xcode handles autocompleted pairs by defaulting to after the first character typed including carriage returns, as mentioned above. Again, this will eventually be configurable via settings.

@knotbin
Copy link
Contributor Author

knotbin commented May 20, 2024

I think this is different from pairs as many times you want to put nothing in the middle (using self closing tags is frowned upon in HTML) and it would be tedious to have to hit space and backspace or type out the entire closing tag, unlike with pairs which are only one character long.

Making the user type space and backspace would make the development experience much less smooth and a closing tag is much longer to type out than a single closing bracket. This is why I believe we should not treat this as a typical pair and Xcode precedent doesn't exist for this scenario

@austincondiff
Copy link
Collaborator

austincondiff commented May 20, 2024

I can see where you are coming from. I appreciate your patience in this discussion and I hope you don't mind the back and forth. Just a few points to consider...

using self closing tags is frowned upon in HTML

While true as of recent for HTML specifically, it is still pretty common to do this in JSX.

I think this is different from pairs as many times you want to put nothing in the middle [...] Making the user type space and backspace would make the development experience much less smooth and a closing tag is much longer to type out than a single closing bracket.

This shouldn't be necessary to get this desired outcome. Just as you can type ] after a [ to close an array for example, you can type </ after completing an opening tag to complete the closing tag and place the cursor at the end.

This could go like this...

  1. <div>|
  2. <div><| (doesn't autocomplete yet because we don't yet know if we will be opening a new tag - <p>)
  3. <div></div>| (it autocompletes now, because now we know the intent is to close the current tag because the last character typed was /)

But, what if the desired line was...

<div><p>Hello world!</p></div>

This could go like this...

  1. <div>|
  2. <div><| (doesn't autocomplete yet because the character typed was a < so we don't know if this will be a closing tag - </div>)
  3. <div><p|</div> (it autocompletes now, because now we know the intent is to open a new tag)

@knotbin
Copy link
Contributor Author

knotbin commented May 20, 2024

This last case you presented is why I believe it's much better to have it work as it currently does. Nesting components is much easier when the closing tag is automatically placed and all of that extra logic you described isn't needed and the dev can just add the nested component without any weird autofill confusion. I feel like people wouldn't know that a closing tag will autofill if it is not appearing, and might just go to type it themselves instead of the nested div. Overall, this feels overly complicated and confusing for something that I believe has the best and most simple solution already implemented.

I think the nesting tabs use case is the biggest argument for the way it currently works, as it is much less confusing for the dev to nest components when they can see the closed component they are nesting in.

The use case of a tag with nothing in between open and close also still feels bad to me, as if the user has to type </, the purpose of the autofill is kind of defeated. I think in this case, it would be very disorienting for webdevs for it to require a space or extra typing to autofill. I think that there is a very good case that tags work very differently from brackets and therefore shouldn't have consistent logic with them.
It feels like the obvious solution is to have it complete immediately to minimize confusion and extra typing, and make nesting tabs more fluid.

@austincondiff
Copy link
Collaborator

austincondiff commented May 20, 2024

It might be interesting to also explore attributes/props. This might look like...

  1. <p|
  2. <p | (we could be auto-closing the tag - do nothing)
  3. <p c|></p> (a letter preceded by a space was entered - we know the intent is to add attributes so we can autocomplete tag but keep the cursor where it is so the user can enter attributes)
  4. <p class="|></p>
  5. <p class="l|"></p>
  6. <p class="large|"></p>

@austincondiff
Copy link
Collaborator

I agree that your current approach is definitely simpler and to be fair, a PR is not the place to explore this. Let's go ahead and merge this and this can be discussed at a later point in time.

Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small things

@knotbin
Copy link
Contributor Author

knotbin commented May 20, 2024

Sorry, should have caught that 😅

@knotbin knotbin requested a review from austincondiff May 20, 2024 21:30
@austincondiff austincondiff merged commit d2f655a into CodeEditApp:main May 20, 2024
2 checks passed
@thecoolwinter thecoolwinter mentioned this pull request May 30, 2024
6 tasks
thecoolwinter added a commit that referenced this pull request May 30, 2024
### Description

Fixes a few bugs with #247 and converts it to use tree-sitter rather
than a regex-based implementation. This should be faster on larger
documents and makes it more robust to edge cases in tag regexes. This
also handles newlines correctly, as the old PR caused the editor to no
longer be able to delete newlines

Also fixes a small bug in the `TreeSitterClient` that caused *every*
query to be dispatched to main asynchronously. This was the cause for a
few visual oddities like flashing colors when changing themes. This also
improves highlighting while scrolling fast as most highlights are
processed synchronously.

- Removes extensions on `NewlineProcessingFilter`
- Cleans up `TagFilter`
  - Moves all newline processing to the one filter
- Use tree-sitter for tag completion, supporting the following
languages: HTML, JSX, TSX
- Adds a few methods to `TreeSitterClient` for synchronously querying
the tree sitter tree.
- Adds a new `TreeSitterClientExecutor` class that the client uses to
execute operations safely asynchronously and synchronously.
- This is extremely useful for testing, as it allows the tests to force
all operations to happen synchronously.
- Adds a check to `dispatchMain` to see if the thread is already the
main thread (meaning no async dispatch)

### Related Issues

* #244 
* Discussion on discord
[Here](https://discord.com/channels/951544472238444645/1242238782653075537)

### Checklist

- [x] I read and understood the [contributing
guide](https://github.com/CodeEditApp/CodeEdit/blob/main/CONTRIBUTING.md)
as well as the [code of
conduct](https://github.com/CodeEditApp/CodeEdit/blob/main/CODE_OF_CONDUCT.md)
- [x] The issues this PR addresses are related to each other
- [x] My changes generate no new warnings
- [x] My code builds and runs on my machine
- [x] My changes are all related to the related issue above
- [x] I documented my code

### Screenshots


https://github.com/CodeEditApp/CodeEditSourceEditor/assets/35942988/8fc559a4-15c9-4b4e-a3aa-57c86c57f7c9


https://github.com/CodeEditApp/CodeEditSourceEditor/assets/35942988/a209b40f-7aa3-4105-aa37-5608e8b4bcdb
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.

✨ Autocomplete angle bracketed components the same way braces are autocompleted
2 participants