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(harpoon)!: remove "j" keymap, make "t" keymap dynamic based on $TMUX #292

Merged
merged 5 commits into from
Jun 22, 2023

Conversation

RayJameson
Copy link
Contributor

I believe it's better when you have to remember one less keybinding.
Hope nobody gets upset because of the removed "j" keymap.
I may add it for compatibility

@RayJameson
Copy link
Contributor Author

RayJameson commented Jun 20, 2023

This also fixes which key preview, by adding an empty function. Before it was <prefix><prefix>, now it's 󱡀 Harpoon, however I didn't figure out how to make it blue color as it should be.

@RayJameson RayJameson force-pushed the main branch 2 times, most recently from f1a4ca7 to 78e877b Compare June 20, 2023 16:17
@RayJameson
Copy link
Contributor Author

RayJameson commented Jun 20, 2023

randomly fixed the error that occurred if user canceled input when using the "go to terminal" thing, also added a new keymap to jump to the index of the mark

@RayJameson RayJameson force-pushed the main branch 4 times, most recently from e555628 to d1c4f7c Compare June 20, 2023 19:39
@RayJameson RayJameson changed the title feat(harpoon): make terminal keymaps dynamic based on $TMUX feat(harpoon)!: make terminal keymaps dynamic based on $TMUX Jun 20, 2023
@luxus
Copy link
Member

luxus commented Jun 20, 2023

anyone else that uses harpoon can confirm that?

@RayJameson RayJameson force-pushed the main branch 3 times, most recently from f8f41d6 to f21f4e2 Compare June 20, 2023 21:57
@RayJameson
Copy link
Contributor Author

RayJameson commented Jun 20, 2023

@luxus on second thought, maybe it's better to leave keymaps unchanged as they do not contradict each other. You can still make :term while using TMUX, maybe someone using it. And in my experience, the require("harpoon.tmux").gotoTerminal(num) doesn't work as intended, I checked the plugin repo and there is at least one open issue that repeats my experience:

I can remove remap part and leave everything else

Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

Hey @RayJameson just a heads up, I went ahead and rebased this on main because there was a bit of standardization that was done for filenames

@owittek
Copy link
Member

owittek commented Jun 22, 2023

This also fixes which key preview, by adding an empty function. Before it was <prefix><prefix>, now it's 󱡀 Harpoon, however I didn't figure out how to make it blue color as it should be.

Could you please elaborate what the empty function fixes exactly? Having any functionality (like an anonymous function) leads to the which-key entry not being colored.

@mehalter mehalter requested a review from a team June 22, 2023 16:46
@RayJameson
Copy link
Contributor Author

RayJameson commented Jun 22, 2023

This also fixes which key preview, by adding an empty function. Before it was <prefix><prefix>, now it's 󱡀 Harpoon, however I didn't figure out how to make it blue color as it should be.

Could you please elaborate what the empty function fixes exactly? Having any functionality (like an anonymous function) leads to the which-key entry not being colored.

You're correct, however, without it it is named "prefix", I guess it's either one or another, unless you know the way, how to make it colored and with the correct description.

How does it look now:
image

How does it look with empty function:

  • with enabled icons
image
  • with disabled icons
image

By the way it is not only this plugin problem, trouble.nvim has similar mapping, and it's also just "prefix", I believe it's every plugin which set keymap without action

Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

A few small comments about more easily setting variables and small typo fix

lua/astrocommunity/motion/harpoon/init.lua Outdated Show resolved Hide resolved
lua/astrocommunity/motion/harpoon/init.lua Outdated Show resolved Hide resolved
lua/astrocommunity/motion/harpoon/init.lua Outdated Show resolved Hide resolved
Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

LGTM!

@mehalter
Copy link
Member

@RayJameson I think the last thing before we merge, is do you think you could come up with a good way to word the fact that there are binding changes from this commit? I think that would take higher precedence than the dynamic changing of the text in the message

@RayJameson RayJameson changed the title feat(harpoon)!: make terminal keymaps dynamic based on $TMUX feat(harpoon)!: remove "j" keymap, make "t" keymap dynamic based on $TMUX Jun 22, 2023
@RayJameson
Copy link
Contributor Author

@mehalter done

@mehalter mehalter merged commit 9bb0747 into AstroNvim:main Jun 22, 2023
@mehalter
Copy link
Member

Thanks for your work on this @RayJameson ! ❤️

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.

4 participants