-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Keep code coloring in search results short text #68699
Keep code coloring in search results short text #68699
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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.
There's a test case to fix, and a few ideas/points in comments.
9e7808f
to
c95068f
Compare
Updated! |
@bors r+ |
📌 Commit c95068f5035c6cecb90ba5a555219d751249739c has been approved by |
@bors r- The Additionally if the summaries for the search index are be rendered to HTML then the text will need to be HTML escaped. HTML rendering should use |
@ollie27 Great catch! We really need the front-end checks to come back... cc @pietroalbini :) |
@ollie27 I fixed the html being added into the titles. However, I don't think we ever use the search index content directly. Do you have a location in mind where it would be the case? Because otherwise, it'll greatly increase the search index size. And I don't really want to put a markdown converter in the front end... |
@GuillaumeGomez I don't know what you mean. The search index summaries and the sidebar titles are generated separately. Currently they both use the |
☔ The latest upstream changes (presumably #69172) made this pull request unmergeable. Please resolve the merge conflicts. |
@ollie27 Sorry, I didn't write that in a very understandable way... To make it simple: with the current version, is this issue fixed? |
No, you appear to be trying to treat a symptom rather than the cause. The issue with this PR is that |
Ok! |
Triaged |
464c3e1
to
d57901d
Compare
I went for the simplest solution (and I think the best too): I only perform this change on the front-end side. Is it good for you too @ollie27 ? |
This will treat any backticks as though they are surrounding code spans but that's not always the case. |
Do you have an example in mind? That'd make the testing simpler. |
Some examples I came up with: /// foo `this is a code span` bar \`this isn't\` baz
pub struct Foo;
/// foo `` this is a code span containing `backticks` `` bar
pub struct Bar; |
So I have to skip if it's "\`" and handle multiple backticks. Makes sense. |
No. The summaries stored in the search index don't contain that information. For: /// foo `this is a code span` bar \`this isn't\` baz
pub struct Foo; the search index contains:
There is no way in JavaScript to know which backticks meant codeblocks and which didn't. As I've said, to do this right the summaries need to be rendered and stored in the search index as HTML. |
@@ -1424,6 +1424,17 @@ function getSearchElement() { | |||
return tmp; | |||
} | |||
|
|||
function colorCode(s) { | |||
var parts = s.split("`"); |
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.
This seems extremely fragile. We should be using the builtin markdown renderer for this
r? @jyn514 |
I agree with |
But then we'll make the search index grows, which I'm trying to avoid. However I agree that doing it on the JS side doesn't look so good. So I really don't know here... |
I don't think they will grow much here |
Another issue is for links: do we want to allow them or not? If they're relative, it's not going to have the expected output, so I guess we'll need to to remove the links there too. |
Yes, strip links |
Marking this as waiting on author since it doesn't require frontend tests to fix. |
So is the plan to go with rendering Markdown to HTML using pulldown? |
Yes. |
Closing in favor of #77686. |
Fixes #32040.
r? @kinnison