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

All my changes (don't merge) #1091

Closed
wants to merge 5 commits into from
Closed

All my changes (don't merge) #1091

wants to merge 5 commits into from

Conversation

trusktr
Copy link
Member

@trusktr trusktr commented Mar 23, 2020

I'm just opening this to start some discussions. This PR has all the changes I made for my fork of Docsify to get my documentation project online with needed functionality.

The plan is that I'll make other pull requests with individual changes (with nice commit messages).

Make pull requests for

@trusktr
Copy link
Member Author

trusktr commented Mar 23, 2020

I've got a lot of conflicts. :D

@trusktr
Copy link
Member Author

trusktr commented Mar 23, 2020

I'll close this so it doesn't distract from valid pull requests, but we can still discuss.

@trusktr trusktr closed this Mar 23, 2020
@@ -1,3 +0,0 @@
{
"prettier.eslintIntegration": true
}
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure why I deleted this file. Maybe I had differing VS Code settings. Personally I don't like to commit IDE files. I'd rather commit config files for tools like prettier, eslint, etc, that everyone must use regardless of IDE; developers should configure their IDE to use the tools (most modern IDEs support most of the common tools).

Copy link
Member

Choose a reason for hiding this comment

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

yeah, no need to commit this. we can remove this file

package.json Show resolved Hide resolved
src/core/config.js Show resolved Hide resolved
src/core/init/index.js Show resolved Hide resolved
if (href === _self.config.homepage) {
href = 'README'
}
href = router.toURL(href, null, router.getCurrentPath())
Copy link
Member Author

Choose a reason for hiding this comment

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

As the comment says, this prevents Docsify from erroneously handling links that are already in Docsify's format. This makes it easy to link to known pages if you already know the URL you want to link to. I think this is related to the next comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember why I needed this. I'll report back after some investigation.

@@ -258,7 +264,7 @@ export class Compiler {
attrs += ` title="${title}"`
}

return `<a href="${href}"${attrs}>${text}</a>`
return `<a docsify-link href="${href}"${attrs}>${text}</a>`
Copy link
Member Author

Choose a reason for hiding this comment

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

I needed a way in my own code to detect Docsify links. The reason is that I have a custom html() hook to modify the markdown rendering and needed to skip links that are already handled by Docisfy:

This is my html hook:

			markdown: {
				// `this` in the following hooks is an instace of Marked Renderer
				renderer: {
					html(html) {
						const matches = html.matchAll(reAnchorWithHref)

						const {linkTarget, router} = vm.compiler

						// if we find an anchor tag with an href attribute
						for (const match of matches) {
							// if the link is a Docsify link generated from markdown, skip it, it is already handled
							if (match[0].startsWith('<a docsify-link')) continue // <--------------------- HERE

							// the result will be one of the three capturing groups from the regex
							let href = match[1] || match[2] || match[3]

							const originalHref = href

							// the first two capturing groups catch single or double quoted values
							const hasQuotes = !!(match[1] || match[2])

							// based on Docsify's Compiler._initRenderer() logic for the markdown "link" hook {{{

							// TODO make some syntax for telling it to ignore the compiling the href
							const ignoreLink = false

							if (
								!Docsify.util.isAbsolutePath(href) &&
								!vm.compiler._matchNotCompileLink(href) &&
								!ignoreLink &&
								// skip hrefs like `#/page?id=section`, which
								// are already in the format Docsify compiles
								// hrefs to
								// TODO move this to router.toURL
								!href.trim().startsWith('#/')
							) {
								if (href === vm.compiler.config.homepage) {
									href = 'README'
								}
								href = router.toURL(href, null, router.getCurrentPath())
							}

							// }}}

							if (!hasQuotes) href = '"' + href + '"'

							html = html.replace(originalHref, href)
						}

						return html
					},

I need to think about if this is the best way to do what I wanted, but I don't remember what problem I needed to solve. I'll need to disable this and see how it breaks my links so I can remember. 😄

Some of the logic is duplicated from other code inside Docsify, which isn't ideal. Let me think of how it can be easier...

Copy link
Member

Choose a reason for hiding this comment

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

for, this I think we can have a use case if we reshape this a little.

like mentioned here in this issue #1088 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Note also this piece of code is where I needed the vm instance that was passed into the $docsify config when it is a function.

like mentioned here in this issue #1088 (comment)

Gotcha, so if it had that feature then I could detect the ID prefix, similar to what I did with the docsify-link attribute.

const vueVM = window.__EXECUTE_RESULT__
vueVM && vueVM.$destroy && vueVM.$destroy()
window.__EXECUTE_RESULT__ = new window.Vue().$mount('#main')
}, 0)
// }, 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes with getScript/executeScript prevents script tags from executing twice. The setTimeout isn't needed, at least as far as I can tell everything works for me fine.

Maybe it was a hack to defer script execution to later so that things in the DOM are ready? If that's the case, users can for example use jQuery.ready() like normal (or etc).

Copy link
Member

Choose a reason for hiding this comment

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

I think it will effect the custom scrip option which are defined in the markdown right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

custom scrip option which are defined in the markdown right

Can you expand on 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.

2 participants