Skip to content

Fix the search error after setting the ID in the title #1159

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
May 14, 2020

Conversation

sy-records
Copy link
Member

Summary

Fix the id set in the title, resulting in wrong link when searching.

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

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

If changing the UI of default theme, please provide the before/after screenshot:

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.

Other information:


  • DO NOT include files inside lib directory.

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.

After tests , the normal cases work fine. 👍
A little problem(maybe it is not a problem), when the id settings like :id=xxx%xxx ( including the %) , it will get this issues #1118 again.
cuz, at thegetAndRemoveConfig()

 .replace(/(?:^|\s):([\w-]+:?)=?([\w-%]+)?/g, (m, key, value) => {

we allowed the % for getting the resizing params , such as ':size=10%'.

this util relates to lots of configurations and makes things broken sometimes. 😅

At the same time, when the id config has some weird characters, this settings would broken also.
If possible, I glad to see this regex in getAndRemoveConfig() will be refined soon, or there would have better solution instead.

@sy-records
Copy link
Member Author

The ':size=10%' of the image is unaffected.

Since I couldn't use local git commits directly, I changed one less place and now I can.

@sy-records sy-records requested a review from Koooooo-7 May 8, 2020 00:31
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.

LGTM.:+1:

@anikethsaha anikethsaha merged commit 6e554f8 into docsifyjs:develop May 14, 2020
@anikethsaha
Copy link
Member

anikethsaha commented May 14, 2020

@sy-records thanks for the PR. 💯

just a small point, we are using angular commit convention so the commit message format should match these guidelines.

@sy-records sy-records deleted the patch-1 branch May 14, 2020 11:55
@sy-records
Copy link
Member Author

okey.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants