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

add option to allow user to configure floating window #71

Merged
merged 6 commits into from
Mar 23, 2023

Conversation

matthewsia98
Copy link
Contributor

Allow users to configure floating win for :JupyniumKernelHover by passing win_options key in setup

require("jupynium").setup({
    win_options = {
        border = "rounded",
    },
})

@kiyoon
Copy link
Owner

kiyoon commented Mar 23, 2023

Thanks for the PR! However, this defeats one principle of this plugin: all settings must be Jupynium's vim plugin version agnostic.
Since this plugin should interact with multiple instances of neovim, sometimes even remote ones over SSH, this plugin operates a little bit in a special way: it should run even without installing the vim plugin (and only using the python package pip install jupynium).

The benefits of this approach are:

  • No need to match the possibly different version of Jupynium plugin installed on each system
  • You can attach to neovim with absolutely no plugin installed. Good for quick usage in some servers that you find it hard to sync your dotfiles.

There are four things that should be modified from here.

  1. Use pcall(require( ... )) to bypass errors when the jupynium module is not found.
  2. I recently made changes to the lines in the PR feat(hover): better highlighting #70. You should rebase it and change lines to markdown_lines
  3. Add the default option to the lua/jupynium/options.lua's M.default_opts as well. You can leave the default option in the helpers.lua file in case the plugin is not installed and the pcall require fails.
  4. Update README about the default configuration. Add some comments about what it does (i.e. it is used for :JupyniumKernelHover)

@kiyoon
Copy link
Owner

kiyoon commented Mar 23, 2023

We can also make the option grouped to the kernel hover just in case in the future there are different types of floating windows we may use, and different options that may be added for hover (like timeout etc.). And name it floating_win_opts to make it more clear what it does.

require("jupynium").setup({
  kernel_hover = {
    floating_win_opts = {
      border = "rounded",
    },
  },
})

@matthewsia98 matthewsia98 force-pushed the configure-floating-win-options branch from 58d7776 to af436ca Compare March 23, 2023 19:55
@kiyoon
Copy link
Owner

kiyoon commented Mar 23, 2023

I'd prefer if we use kernel_hover.floating_win_opts than floating_win_opts.hover because there might be more settings for hover.

Also, can you add max_width = 84 in the default as well?

lua/jupynium/options.lua Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kiyoon
Copy link
Owner

kiyoon commented Mar 23, 2023

I think it's ready to be merged if you have no more comments!

@matthewsia98
Copy link
Contributor Author

I think it's ready to be merged if you have no more comments!

all good from me

@kiyoon kiyoon merged commit 01162b0 into kiyoon:master Mar 23, 2023
@kiyoon
Copy link
Owner

kiyoon commented Mar 23, 2023

I really appreciate this PR!

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