-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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: GitHub style callouts #2487
Open
jhildenbiddle
wants to merge
15
commits into
develop
Choose a base branch
from
github-callouts
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
f9950e0
Add support for GitHub style callouts
jhildenbiddle f9a9869
Update docs and migrate to new syntax
jhildenbiddle db423b6
Update test snapshot
jhildenbiddle 6cf5f36
Add tests for new callout syntax
jhildenbiddle 72627ed
Merge branch 'develop' into github-callouts
Koooooo-7 ee73d59
Merge branch 'develop' into github-callouts
Koooooo-7 81c1dd0
Merge branch 'develop' into github-callouts
Koooooo-7 26240d3
update: sync the callout
Koooooo-7 3501b04
Merge branch 'develop' into github-callouts
Koooooo-7 64f5a92
Merge branch 'develop' into github-callouts
Koooooo-7 543e35d
Merge branch 'develop' into github-callouts
Koooooo-7 fea3b1b
Merge branch 'develop' into github-callouts
Koooooo-7 88e32ff
Merge branch 'develop' into github-callouts
Koooooo-7 9a27faa
Merge branch 'develop' into github-callouts
Koooooo-7 3607c8a
Merge branch 'develop' into github-callouts
Koooooo-7 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,12 @@ | |
disabled | ||
/> | ||
|
||
<!-- Prism Theme --> | ||
<!-- <link | ||
rel="stylesheet" | ||
href="//cdn.jsdelivr.net/npm/[email protected]/themes/prism-twilight.min.css" | ||
/> --> | ||
|
||
sy-records marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<!-- Site styles --> | ||
<style> | ||
/* Plugin: Carbon Ads */ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think it would be great to add a
console.warn
when the respective piece of code runs, so people will know. We could also tell people to use major versions in deprecation messages, to start to get versionless people knowing what will be happening, so they can prepare. Wdyt?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.
Easy enough. Happy to add it. I may consolidate console deprecation warnings as well so they are easier to track and the output can be standardized.
This one may introduce some false positives simply because of the many different ways Docsify can be loaded, but I think a simple search for a
<script>
tag with asrc
value that matches the following criteria could work:.com
,.net
,.org
, etc.)\\docsify(\.min)?\.js
@version/
and/version/
CDN URLs)This would match the following URLs and trigger the console warning:
The following URLs would be ignored:
FWIW, I may opt to implement this in a separate PR. Replying here for convenience. :)
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.
As per the experience of I work the compatibility release, it seems too complicated for us to handle too much cases.
If we can ensure that we notify as many people as possible who will be affected, notifying additional people who may not be impacted is acceptable.
So I suppose we could make the
versionDetector
simply like this:Firstly, We check some common CDN providers (unpack, jsdelivr) import path format with versions.
Then, for other importing cases, we only try to find the
docsify.min.js
and thedocsify.js
content. No matter what the path folder/5/
nor the domain is, cos we don't know how users host docsify on which server, which place, which ip, which domain, which CDN...As long as it retrieves new docsify resources without versioning, it gets caught by theversionDetector
.When the site contains either resources name of them (
docsify.min.js
/docsify.js
), we throw out a warning such as:Conclusion:
If users import the resource online (host site on a domain or a raw IP are both okay ), they would get the warning.
If users deploy docsify internally or locally, they won't get a docsify warning, cos they won't get any docsify update also. version control doesn't matter for them. Once they manually upgrade docsify with
versionDetector
, they would get the warning then.Besides, we provide a
versionDetector
config to manually suppress the warning and checking, when they don't care about any breaking changes or the resource import place is not a generic resources versioning control path (e.g. https://dontblameme.com/libs/update/20240401/docsify.min.js).or am I too straightforward?