Skip to content

feat: indent guides #107

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

Closed

Conversation

danilshvalov
Copy link
Contributor

I'm sorry for the delay. There wasn't enough time to finish.

@cseickel
Copy link
Contributor

cseickel commented Feb 1, 2022

I'm sorry for the delay. There wasn't enough time to finish.

No problem at all, we all have lives outside of open source contributions! Thanks for submitting, I'll review soon.

@danilshvalov
Copy link
Contributor Author

I have simplified the code.

@cseickel cseickel linked an issue Feb 2, 2022 that may be closed by this pull request
@cseickel
Copy link
Contributor

cseickel commented Feb 2, 2022

Thanks for making those changes. While testing ti out I realized there is one major item we forgot about:

  1. We add a config option for a special component called "indent_renderer" which follows the interface of a component but is at the global config level.

I think it needs to be changed to this method of configuration to make it easier to configure and prevent breaking changes. With this strategy, the indent_renderer would be at the top level of the config and be implicitly put at the beginning of all renderers.

If you are short on time I can make these changes before merging, just let me know.

@danilshvalov
Copy link
Contributor Author

I have added a global configuration implementation. Now the user can globally configure any component. If the user manually changed the component, the configuration will be overridden (local configuration is more significant than the global one).

For example, user can configure icons:

require("neo-tree").setup({
      icon_renderer = {
          folder_closed = "-",
      },
     -- other configurations
})

@cseickel
Copy link
Contributor

cseickel commented Feb 2, 2022

I really like the idea of having a global config and we should definitely do that.

What this still doesn't address is the fact that it is a breaking change to take an implicit component and turn it into an explicit one. Everyone that has applied any customization to their renderers will suddenly lose indents with this update,

To prevent that, we have to either:

  1. Remove it from the individual renderers completely and just pull the settings from global config instead. This is what happens now.
  2. In setup(), check for the existence of the "indent" component in each config, and inject it only if it doesn't already exist.

Number two fits better with what you have now. It can probably happen in the add_global_components_config function. I would say test for the existence of "indent" in the loop and inject it in the first position only if it wasn't seen. It is is possible someone may have it but not in the first position because they want to have something like diagnostics or git_status all the way on the left before the indent.

@cseickel
Copy link
Contributor

cseickel commented Feb 6, 2022

Hey @danilshvalov, I'm going to finish this up and merge it. The only outstanding issue was ensuring the component is always implicitly added even if the users have a custom renderer without an indent component.

@cseickel cseickel closed this Feb 6, 2022
@cseickel
Copy link
Contributor

cseickel commented Feb 7, 2022

Oops, didn't mean to close it.

@cseickel cseickel reopened this Feb 7, 2022
@cseickel
Copy link
Contributor

cseickel commented Feb 7, 2022

Content moved to #131 so I could squash and rebase.

@cseickel cseickel closed this Feb 7, 2022
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.

New feature: indent guides
2 participants