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

Convert {docsify-ignore} and {docsify-ignore-all} to HTML comments #1318

Merged
merged 3 commits into from Jul 30, 2020
Merged

Convert {docsify-ignore} and {docsify-ignore-all} to HTML comments #1318

merged 3 commits into from Jul 30, 2020

Conversation

equinusocio
Copy link
Contributor

@equinusocio equinusocio commented Jul 29, 2020

Ref and close #441
This should also close #767

Summary

This PR prevents {docsify-ignore} and {docsify-ignore-all} to be rendered inside other markdown engines like on Github.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Repo settings
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Tests

$ mocha ./test/**/*.test.js


  router/history/base
    ✓ toURL without relative path
    relativePath true
      ✓ toURL
      ✓ toURL with double dot
      ✓ toURL child path
      ✓ toURL absolute path

  Docsify public API
    ✓ global APIs are available (183ms)
    Docsify config function
      ✓ allows $docsify to be a function (201ms)
      ✓ provides the hooks and vm API to plugins

  render
    ✓ important content (tips)
    lists
      ✓ as unordered task list
      ✓ as ordered task list
      ✓ normal unordered
      ✓ unordered with custom start
      ✓ nested
    image
      ✓ regular
      ✓ class
      ✓ id
      ✓ no-zoom
      size
        ✓ width and height
        ✓ width
    heading
      ✓ h1
      ✓ h2
      ✓ h3
      ✓ h4
      ✓ h5
      ✓ h6
    link
      ✓ regular
      ✓ linkrel
      ✓ disabled
      ✓ target
      ✓ class
      ✓ id

  router/util
    ✓ resolvePath
    ✓ resolvePath with dot
    ✓ resolvePath with two dots


  35 passing (1s)

@vercel
Copy link

vercel bot commented Jul 29, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/n5ph96a7z
✅ Preview: https://docsify-preview-git-fork-equinusocio-develop.docsify-core.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 29, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 936cab3:

Sandbox Source
docsify-template Configuration

sy-records
sy-records previously approved these changes Jul 29, 2020
Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

LGTM.

Should we be compatible with previous versions? @anikethsaha

@sy-records sy-records requested review from anikethsaha and a team July 29, 2020 10:09
@equinusocio
Copy link
Contributor Author

equinusocio commented Jul 29, 2020

LGTM.

Should we be compatible with previous versions? @anikethsaha

If we want to solve #767 whit this PR, i think should not be compatible.

@Koooooo-7
Copy link
Member

I think it should be backward compatibility. we'd better keep current congratulation and allowed the new one.
If users haven't used the specific version of docsify, it would cause potential troubles for them. we should make it deprecated, not just remove it directly rn.

@equinusocio
Copy link
Contributor Author

equinusocio commented Jul 29, 2020

If users haven't used the specific version of docsify, it would cause potential troubles for them. we should make it deprecated, not just remove it directly rn.

This is how dependencies work. If you don't specify a version you will get the last version, including any major releases that always include breaking changes. Users should know how how to use it.

Who is using @4 will not get this fix since i think it will be shipped with v5 (?). Who is using docsify without a specified version will automatically get the v5 with their relative breaking changes, including this one (personally, i think this is not a good usage)

Assuming this fix will shipped with v5:

//cdn.jsdelivr.net/npm/docsify@4/lib/docsify.min.js = No fix
//cdn.jsdelivr.net/npm/docsify@5/lib/docsify.min.js = Fix included
//cdn.jsdelivr.net/npm/docsify/lib/docsify.min.js = All releases, including breaking, fix included

Koooooo-7
Koooooo-7 previously approved these changes Jul 29, 2020
Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

This is how dependencies work. If you don't specify a version you will get the last version, including any major releases that always include breaking changes. Users should know how how to use it.

Who is using @3 will not get this fix since i think it will be shipped with v4 (?). Who is using docsify without a specified version will automatically get the v4 with their relative breaking changes, including this one (personally, i think this is not a good usage)

make sense, LGTM.

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Can we have some test for this change ?

@equinusocio
Copy link
Contributor Author

Done

Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

cool 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants