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(markdown-and-latex): add markmap.nvim #229

Merged
merged 3 commits into from
May 26, 2023
Merged

feat(markdown-and-latex): add markmap.nvim #229

merged 3 commits into from
May 26, 2023

Conversation

Zeioth
Copy link
Contributor

@Zeioth Zeioth commented May 26, 2023

No description provided.

@Uzaaft
Copy link
Member

Uzaaft commented May 26, 2023

Please make sure that your PR title follows the semantic PR spec

@mehalter mehalter changed the title Added: Markmap.nvim feat(markdown-and-latex): add markmap.nvim May 26, 2023
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.

Made a few modifications to make it more generally accessible to AstroNvim users. Thanks for the contribution!

@mehalter mehalter merged commit 49f60ac into AstroNvim:main May 26, 2023
@Zeioth
Copy link
Contributor Author

Zeioth commented May 26, 2023

@mehalter just one thing:

npm install -g markmap-cli

will fail because -g require sudo permissions.

yarn global add markmap-cli

Allow to install globally just for the user without the need of sudo permissions. That's why I use it. I don't think there is a way around it.

@mehalter
Copy link
Member

Sorry about that! I just fixed it on main: f001b6a

@Uzaaft
Copy link
Member

Uzaaft commented May 26, 2023

@mehalter @Zeioth That's not correct.
npm install -g markmap-cli would fail only if you've setup node and npm uncorrectly. I think we should revert f001b6a

@Uzaaft
Copy link
Member

Uzaaft commented May 26, 2023

image
See here. Worked fine with npm for me.

@mehalter
Copy link
Member

mehalter commented May 26, 2023

  1. That is not necessarily the case @Uzaaft it will work if you install npm locally. If you use a system package manager to install it then -g will require sudo and default to the full system (and installing npm with a system package manager shouldn't be considered incorrect). I have looked into this and there is no better way to do this for the general use case. Maybe what we should do is remove the system installation and just say it requires markmap to be in your path like other plugins do. If someone wants to extend it to install with their installer of choice they can. What do you think of that @Zeioth
  2. That commit does more than just changing that line (file/folder renames) so we cannot just simply revert that commit.

@Uzaaft
Copy link
Member

Uzaaft commented May 26, 2023

  • That is not necessarily the case @Uzaaft it will work if you install npm locally. If you use a system package manager to install it then -g will require sudo and default to the full system. I have looked into this and there is no better way to do this for the general use case. Maybe what we should do is remove the system installation and just say it requires markmap to be in your path like other plugins do. If someone wants to extend it to install with their installer of choice they can. What do you think of that @Zeioth

I've installed npm with pacman on my arch machine, and there, I've had to setup npm correctly to setup non-sudo usage. See the official docs: https://docs.npmjs.com/resolving-eacces-permissions-errors-when-installing-packages-globally

Either way, npm recommends you to use node version manager in their docs, I think we should stick to npm, since most people are likely to have that.

@Zeioth
Copy link
Contributor Author

Zeioth commented May 26, 2023

Thank you so much for going through the effort of checking that out guys. I've documented it here. So whatever you decide will be fine.

@jay-babu
Copy link
Contributor

Can't mason just be used to manage the installation of this?

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