8000 Add plantuml support by kazuph · Pull Request #57 · previm/previm · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@kazuph
Copy link
Contributor
@kazuph kazuph commented Apr 29, 2016

No description provided.

@kannokanno
Copy link
Collaborator

すいません遅くなりました。確認致します!

@kannokanno
Copy link
Collaborator

ありがとうございます!
少し気になる点がありましたので、ご検討お願い致します。

なお、Javascript API Client Codeを元にしたものと解釈してコメントしています。

スコープを極力小さくして欲しい

plantuml.jszip_deflate.js共に全ての変数/関数がグローバルの名前空間に定義されます。

  • グローバル定義の必要がないものがほとんどだと思います
  • plantuml.jsloadPlantUMLzip_deflate.jsは上記の通り対応すればdeflateだけグローバルに公開されていれば良いかと思います

zip_deflate.jsの参照元を記載する

上記の通り、zip_deflate.jsは https://github.com/johan/js-deflate/blob/master/rawdeflate.js かと思っています。
もしそうであれば、zip_deflate.js内にその旨をコメントして頂きたいです。
johan/js-deflateにはライセンス定義はないので不要かもしれませんが、マナーとして載せておきたいです。

ライブラリ側はmin.jsにしたい

速度向上のために(特にzip_deflate.jsが)min.jsになると嬉しいです。

@kyoh86
Copy link
Contributor
kyoh86 commented Oct 3, 2017

@kazuph こちら対応予定は無いですか? 💓

@mbeko
Copy link
mbeko commented Apr 14, 2018

I don't speak Japanese, so I hope we can use English.

I would also appreciate a Markdown preview with PlantUML diagrams. Besides the problems mentioned by kannokanno, I would like to have a version which doesn't contact an external server because Vim should be usable offline.

This is difficult to achieve because you can't execute local binaries from Javascript. The PlantUML server could be launched locally, but I don't think it's good to do this every time Previm is used.

So I would propose to auto-refresh the generated HTML from Vimscript from where the PlantUML jar could be called locally. But this could be a large change to the whole plugin. Do you think it's worth following that idea?

@mattn mattn mentioned this pull request Oct 5, 2018
@mattn
Copy link
Member 82FF
mattn commented Oct 5, 2018

Rebase master #113. So closing this.

@mattn mattn closed this Oct 5, 2018
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.

5 participants

0