antonk52 / cssmodules-language-server

autocompletion and go-to-defintion for cssmodules
MIT License
54 stars 8 forks source link

Support LSP actions in classes with dashes #25

Closed moliva closed 2 months ago

moliva commented 3 months ago

Hi, first of all thanks for working on this LSP as it's been quite useful while developing and consuming CSS modules from TS code. Sorry in advance as I have not created an issue previously for this and went ahead with this PR.

Issue

When using CSS class names with dashes (e.g. .class-name) and camelCase = false configured some feats don't work as expected:

This PR includes:

antonk52 commented 3 months ago

Hi and thank you for adding support for class names with dashes. I have never works in a code base that used class names with dashes thus this case was overlooked in the passed.

I have tested it and it works as advertised.

I think since this is being added, it makes sense to make [ char also a character that triggers a completion. This can be done here

https://github.com/antonk52/cssmodules-language-server/blob/main/src/connection.ts#L47

This change would require making a few more changes as current code assumes that . is the only completion trigger character. Let me know if you are interested in doing this in the same PR.

Thank you for fixing the typos :)

moliva commented 2 months ago

Hey, sure! Let me take a look at it!

moliva commented 2 months ago

Hi again! I just committed the changes to add [ as another trigger (also added ' and " to avoid the editor from cancelling the completions prompted).

It became a bit trickier to reconcile cases for the configuration of the completion item (mostly when having wrapping quotes or brackets), but everything seems to work as expected on my end, replacing characters when they are already present in the file content (after the position of the cursor).

Please let me know your thoughts, if you want to go ahead with these changes or prefer to revert to the previous version and move this to another PR.

antonk52 commented 2 months ago

Hi. I see this significantly bumped the PR complexity. Sorry for not foreseeing it. Do you mind rolling back the changes regarding the [ trigger character. I will merge this PR and then we can continue in a new PR regarding adding [ as a trigger character?

That's a solid work figuring out the quirks, great job!

moliva commented 2 months ago

Sure, I think it makes sense 😅

Will go ahead and roll those last changes back and create a new PR. We can continue the discussion there.

Thank you!

moliva commented 2 months ago

Done with the rollback!

antonk52 commented 2 months ago

@moliva This pr is merged. Let me know if you want to open a new PR that adds trigger character [

moliva commented 1 month ago

@moliva This pr is merged. Let me know if you want to open a new PR that adds trigger character [

Hey! Sorry, I've just come back from a trip. I've just created this PR, if you want to check it out