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

fix: markdown load slow #307

Merged
merged 3 commits into from
Mar 19, 2024
Merged

fix: markdown load slow #307

merged 3 commits into from
Mar 19, 2024

Conversation

LiusCraft
Copy link
Contributor

No description provided.

@CarlJi
Copy link
Member

CarlJi commented Mar 15, 2024

/assign @nighca

@@ -642,7 +641,9 @@ <h1 class="my-4 text-3xl font-extrabold leading-tight text-gray-900">
var htmlStr = document.documentElement.outerHTML;

// get article toc
getArticleToc();
// markdown load slow, so we need to timeout get toc
setTimeout(getArticleToc,200)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有没有可能通过 MarkdownViewer 暴露的事件感知 markdown load 完成的?

@@ -23,7 +23,7 @@
<link href="https://fonts.cdnfonts.com/css/inter" rel="stylesheet" />
<link rel="preconnect" href="https://fonts.proxy.ustclug.org" />
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
<link href="https://fonts.proxy.ustclug.org/css2?family=Noto+Sans+SC&display=swap" rel="stylesheet" />
<!-- <link href="https://fonts.proxy.ustclug.org/css2?family=Noto+Sans+SC&display=swap" rel="stylesheet" /> -->
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没有用的话就直接删掉?特意注释并保留的话就在旁边说明下原因?

@LiusCraft LiusCraft force-pushed the fix/article-view branch 2 times, most recently from 498ce6a to c12677d Compare March 18, 2024 03:27
@LiusCraft LiusCraft requested a review from nighca March 18, 2024 03:30
@@ -631,17 +632,16 @@ <h1 class="my-4 text-3xl font-extrabold leading-tight text-gray-900">
MarkdownViewer: window.GoplusMarkdown.MarkdownViewer
},
mounted() {
// set markdown change callback function
// look cmd/gopcomm/yap/markdown-vue/src/components/MarkdownViewer.vue
window.mdChange=getArticleToc
Copy link

@nighca nighca Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

通过 window.xxx 作为事件 handler 不是常规的做法,也有明显的问题

这里 MarkdownViewer 是个 vue component,可以通过 vue component 的事件来做,比如把 getArticleToc 添加到上边 methods 里,然后在模板中绑定事件:

<markdown-viewer :md="article.Content" style="height: auto;" @change="getArticleToc"></markdown-viewer>

对应地在 MarkdownViewer 中去 emit change 事件即可;

详见 https://vuejs.org/guide/components/events.html#component-events

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我最开始是这样去做的,它无法生效

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

那先保留 window.mdChange 的做法,然后在这里注释记个 TODO 吧

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


var cherrInstance = null

// this is markdown change callback
// TODO: In order to change the callback function by article_yap.html, this is the worst way to do this.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: TODO 中说明下要“do”什么会更好。这边只说了问题,没说要做什么

@qiniu-ci
Copy link

The PR environment is ready, please check the PR environment

[Attention]: This environment will be automatically cleaned up after 6 hours, please make sure to test it in time. If you have any questions, please contact the author of the PR or the community team.

@CarlJi CarlJi merged commit 89e7dbd into goplus:dev Mar 19, 2024
2 checks passed
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.

None yet

4 participants