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

feat: highlight the current file in the menu #394

Closed
wants to merge 1 commit into from

Conversation

willothy
Copy link

@willothy willothy commented Dec 9, 2023

Adds highlighting to the current file in the menu, and removes the old matchadd method of doing so which did not work.

fixes #384

@tris203
Copy link

tris203 commented Dec 9, 2023

I was also looking at a PR for this.

The only issue I can flag, is that in config.lua the path is normalised a different way using a normalise_path function.

i think that for consistency, either that function should be moved to utils and used in both scripts. or the config file should be modified to use the plenary.path:absolute() method too.

@willothy
Copy link
Author

willothy commented Dec 9, 2023

I was also looking at a PR for this.

The only issue I can flag, is that in config.lua the path is normalised a different way using a normalise_path function.

i think that for consistency, either that function should be moved to utils and used in both scripts. or the config file should be modified to use the plenary.path:absolute() method too.

I agree that it would be ideal for them to be the same, not sure why I did that haha. If you want to work on this I'll close this PR and you can move forward with yours, otherwise I'll make that change :)

@tris203
Copy link

tris203 commented Dec 9, 2023

Happy to do either.

We should probably get input from @ThePrimeagen on which would be preferred. I think to use the : absolute() class in both

I also think that potentially this should be behind an option to either start at the top of the list or follow the file down the list

@willothy
Copy link
Author

willothy commented Dec 9, 2023

Happy to do either.

For sure! You seem to have more ideas about what you want from this, so why don't you do it.

We should probably get input from @ThePrimeagen on which would be preferred. I think to use the : absolute() class in both

I think the choice to use normalized paths was intentional so I'm not sure. Either way there's going to be some conversion from absolute to normalized though, since the list items shouldn't be displayed as absolute paths. Agreed with getting input.

I also think that potentially this should be behind an option to either start at the top of the list or follow the file down the list

I like that idea, would definitely use it.

@willothy willothy closed this Dec 9, 2023
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