-
Notifications
You must be signed in to change notification settings - Fork 15
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
markdown component #100
markdown component #100
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这几个作为构建结果的文件提交进来是预期的吗?
|
||
<script type="importmap"> | ||
{ "imports": { | ||
"vue": "https://cdnjs.cloudflare.com/ajax/libs/vue/3.2.41/vue.esm-browser.prod.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个是必要的吗?如果必要,可能是有问题的,因为这意味着同时在使用两个不同版本的 vue(一个由 global var 方式引入,一个通过 module import 引入)
} | ||
} | ||
|
||
export {MarkdownEditor} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么 MarkdownView
是 default export,而 MarkdownEditor
是 named export?
@@ -78,6 +568,7 @@ axios.defaults.baseURL = 'http://localhost:8080/'; | |||
|
|||
// import "https://goplus.org/_next/static/widgets/code.85827e18ab6a0fa63bdc.js" | |||
export default { | |||
name: "MarkdownViewer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
感觉我们对 MarkdownView
vs MarkdownEditor
的定位理解不一致,我们语音对下吧
output: { | ||
globals: { | ||
vue: "Vue", | ||
Antd: "Antd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib 并没有用到 Antd?
@@ -124,7 +615,7 @@ axios.defaults.baseURL = 'http://localhost:8080/'; | |||
} | |||
}, | |||
mounted() { | |||
this.initCherryMD() | |||
initCherryMD() | |||
this.init_req() | |||
|
|||
let t = "https://goplus.org/_next/static/widgets/code.85827e18ab6a0fa63bdc.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里 widget URL 写死是有问题的,我不知道你有没有看 https://github.com/goplus/www/blob/master/goplus.org/widgets/README.md 这边的文档
We created a widget loader & widget files when building widgets. The loader file's location will be constant: https://goplus.org/widgets/loader.js and the file will not be cached by browser. 3rd-party sites will insert it in page and tell which widgets they want to load. The loader will load corresponding widget files for them.
The widget files defines custom elements on page, which makes HTML tags like work. Widget files' location changes with widget content changing (content-based addressing). But the loader will always know the correct location for each widget, which makes it possible to cache widget files' content locally - actually we will tell browsers to cache widget file content as long as possible.
当后续 goplus-code 这个 widget 迭代更新时,对应的 widget URL 会变更;而这里会没法跟着更新
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这是 goplus-code 的代码吗?是的话为啥会出现在这里?
} | ||
} | ||
|
||
export {MarkdownEditor} | ||
</script> | ||
|
||
<style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib 的 style 似乎包含了不该由它包含的内容?比如 body { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个文件提交的目的是什么?
import vue from '@vitejs/plugin-vue' | ||
|
||
// import { fileURLToPath, URL } from 'node:url' | ||
import Antd from 'ant-design-vue'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个 import 是做什么?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
既然这几个文件提交进来不是预期的,应该删掉?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个文件是要的?
@@ -0,0 +1 @@ | |||
export { default as MyCom1 } from "./MyCom1Install"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
按现在的 vite config 看,我们打包的是这个 lib.js
,但是它对应的 ../src/components/MyCom1.vue
并不存在,这是跑得起来的?
cmd/gopcomm/yap/component/demo.html
Outdated
|
||
|
||
<script type="module"> | ||
import GoplusMarkdown, {MarkdownEditor} from './GoplusMarkdown.js'; // vue组件与编辑组件 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个接口不符合预期,预期的接口是这样的:
// GoplusMarkdown: 我们拆分的模块,它提供 markdown 相关的功能(在标准 markdown 基础上集成了 goplus 的特有逻辑),包括编辑、展示等
// MarkdownEditor: GoplusMarkdown 这个模块对外提供的 markdown 编辑组件,用于满足对 markdown 进行编辑的需求
// MarkdownViewer: GoplusMarkdown 这个模块对外提供的 markdown 展示组件,用于满足对 markdown 进行展示的需求
import { MarkdownEditor, MarkdownViewer } from './GoplusMarkdown.js'
// 界面上嵌入 markdown 编辑器
<MarkdownEditor></MarkdownEditor>
// 界面上嵌入 markdown 内容进行展示
<MarkdownViewer></MarkdownViewer>
如我们之前提到的,如果觉得某个需求(比如展示)很简单,可以通过非 vue 组件的方式来提供功能,那也是可以的,比如我们提供一个渲染函数而不是一个 vue 组件来满足展示的需求:
import { renderMarkdown } from './GoplusMarkdown.js'
// 界面上嵌入 markdown 内容进行展示
let rendered = renderMarkdown(markdownContent)
注意这里我们可能就不会把它命名为 MarkdownViewer
,因为它只是一个渲染 markdown 的函数;不过我还是觉得做成一个 vue 组件会好一点,因为长期来看一个 vue 组件内部可以做的事情更多
现在你的接口的问题是:
- goplus-markdown 本身的定位是模块(提供 markdown 相关的功能),把它用来命名一个组件的话,其定位是模糊的
- 一般情况下,一个库没必要提供多个 entry file(除非有明确的好处),它会增加使用方跟维护方工程上的复杂度;以及这里又引入了一个 markdown-engine 的概念,我看不出新增一个概念的意义(这个概念对使用方是可感知的,概念越多对使用方的理解成本越高)
另外建议明确一下,这个库对外提供的会是 UMD 还是 ESM 格式,还是都提供
// }, | ||
lib: { | ||
// entry: 'src/components/MarkdownEngine.vue', | ||
entry: 'lib/lib.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我没理解错的话,现在提交的 vite config 并不能构建出你的 demo.html 所依赖的内容
<script> | ||
import CherryEngine from 'cherry-markdown/dist/cherry-markdown.engine.core.esm'; | ||
|
||
const MarkdownViewer = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如上面评论中提到的,建议 MarkdownViewer
也定义为一个 vue 组件,其内部实现可以像这里一样很简单,即构造 cherry engine 然后 makeHtml,把结果展示出来即可;而不是暴露一个 { convertMdToHtml: function() {} }
这样的 object
你现在的做法很奇怪:MarkdownEngine 是以一个 vue 组件的形式去定义的,但是使用者并不把它当成一个 vue 组件去使用;要么像上面提到的,就做成一个 vue 组件,使用方直接把它当成一个 vue 组件去使用(推荐这样),用么就不要以一个 vue 组件的形式去定义。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
目前模块的更新生效是依赖维护者在本地手工完成的,后续
- 把这个工作流自动化
- 添加该模块维护的说明文档
二者择一
Extract the markup as a component