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

Extend config to allow setting custom icon and highlight #18

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

ttytm
Copy link
Contributor

@ttytm ttytm commented Apr 24, 2024

The PR extends the config and adds type annotations. With the update it will be possible to easily customize the sign via the config. It can also be a base for further extensions.

require("bookmarks").setup({
	sign = { icon = "" },
})

@LintaoAmons
Copy link
Owner

Hi Tt,

Thanks for making this PR, and I would love to add this function to the plugin.

I have some comments here, if you have some time can take a look, or I could do it later when I got some time~

  1. I don't think we need make this name BookmarksNvimSign to be configurable.
  2. Maybe makes the configuration more user understandable. Like:
{
  icon = "",
  icon_color = "red",
  line_font_color = "blue",
  line_bg_color = "red"
}
  1. change the README accordingly

@ttytm
Copy link
Contributor Author

ttytm commented Apr 27, 2024

  1. I don't think we need make this name BookmarksNvimSign to be configurable.
    Good point about the BookmarksNvimSign, agreed .
  1. Maybe makes the configuration more user understandable. Like:
{
  icon = "",
  icon_color = "red",
  line_font_color = "blue",
  line_bg_color = "red"
}

Personally, I would keep the config for the sign logically structured in a table instead of using a prefix. When extending the config it would could get unappealing quickly.

E.g., a feature I thought of is that it would be cool to be able to have multiple signs. E.g. when setting a mark without a note, and when annotating something with a mark.

2024-04-27_23-51

I updated the PR be easily extensible for such potentials

  1. change the README accordingly

Updated the readme. The commit includes an auto-formatting change. Let me know if this should be reverted.

@LintaoAmons LintaoAmons merged commit 42371a7 into LintaoAmons:main Apr 29, 2024
1 check passed
@ttytm ttytm deleted the sign-config branch April 29, 2024 13:32
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