-
-
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: search ignore diacritical marks #1434
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/fvsz0mwtj |
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 9668613:
|
Unfortunately, normalize does not support IE. Microsoft is also about to discontinue support for IE 11, do we still need to be IE compatible? cc @docsifyjs/reviewers |
@sy-records -- Given the near-universal support of We should verify that this |
I don't have IE11 to testing 😥 If we use this, do we want all users who need to search to add this |
I can test in IE11 once the PR is ready. As for the |
@jhildenbiddle done |
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.
Apologies for the slow response on this, @sy-records.
Verified working in modern browsers. Also verified working with and without polyfill in IE11. Great!
I did notice a few issues:
-
Search results are not displayed with their original diacritical marks. Notice in the screenshot below that the page contains the string "Let's go to café today". A search for "cafe" (no diacritical marks) matches "café" as it should, but the search result displayed under the search field shows the normalized version ("cafe") instead of the original text ("café").
-
Notice in the screenshot below that searching for "let" displays the matching search result text as "...Let's go to a cafe t...", but searching for "cafe" displays "...#39;s go to a cafe t...". That "#39;" looks like an incorrectly rendered HTML entity, but I haven't looked at the code to verify.
-
It looks like apostrophes break the search result highlighting.
FYI: Docsify is currently broken in IE11 due to issues with marked and the carbon ads plugin. I've already submitted fixes for marked (markedjs/marked#1877) and will address carbon ads soon. My tests in IE11 were done with a patched version of marked.
3c38fb1
to
6d0a1bf
Compare
@jhildenbiddle Thanks for the test. The third one is solved.
|
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.
one small suggestion.
Otherwise great stuff.
THanks @sy-records
}; | ||
await docsifyInit(docsifyInitConfig); | ||
await page.fill('input[type=search]', 'documentacion'); | ||
await expect(page).toEqualText('.results-panel h2', 'Que es'); |
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.
is it possible to show the accent in the result ?
sorry if I missed any previous discussion regarding this. LMK
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 don't have a good idea how to do it...
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.
Looks like @sy-records got 2 of the 3 I listed. Perhaps we should open a new issue regarding missing diacritical marks in the search results so we can merge and release this PR.
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.
done. #1491
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.
Created a new issue for missing diacritical remarks in search results (#1492).
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.
LGTM
Summary
search ignore diacritical marks
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)resolve #1405
You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.