-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
[Search] Inject id and class attr into individual result #3142
base: develop
Are you sure you want to change the base?
[Search] Inject id and class attr into individual result #3142
Conversation
If requested, I can submit a PR for documentation later. |
? '<a class="result" href="' + result[fields.url].replace(/"/g, '') + '">' | ||
: '<a class="result">'; | ||
? '<a href="' + result[fields.url].replace(/"/g, '') + '" ' | ||
: '<div '; |
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 is a breaking change as existing projects could rely on always assuming an a
tag. And an anchor tag without href attribute is still valid HTML
|
||
html += result[fields.url] | ||
? '</a>' | ||
: '</div>'; |
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.
See above, this should be reverted to always create an achor a
tag
? ' id="' + result[fields.id] + '" ' | ||
: ''; | ||
|
||
html += result[fields.classes] !== undefined |
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.
we should rename the fields property to class
instead of classes
? ' id="' + result[fields.id] + '" ' | ||
: ''; | ||
|
||
html += result[fields.classes] !== undefined |
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.
we should rename the fields property to class
ins tead of classes
? '<a class="result" href="' + result[fields.url].replace(/"/g, '') + '">' | ||
: '<a class="result">'; | ||
? '<a href="' + result[fields.url].replace(/"/g, '') + '" ' | ||
: html += '<div '; |
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.
Not needed and a breaking change, keep using the anchor tag
|
||
html += result[fields.url] | ||
? '</a>' | ||
: '</div>'; |
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.
See above and revert. Keep using the anchor tag
@@ -1459,6 +1457,8 @@ | |||
results: 'results', // array of results (standard) | |||
title: 'title', // result title | |||
url: 'url', // result url | |||
id: 'id', // HTML 'id' attribute | |||
classes: 'classes', // Classes specific to each result to add to the HTML 'class' attribute. |
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.
We should name the property class
instead of classes
: ''; | ||
|
||
html += result[fields.classes] !== undefined | ||
? ' class="result ' + result[fields.classes] + '">' |
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.
needs to be sanitized (double quotes removed)
? ' class="result ' + result[fields.classes] + '">' | |
? ' class="result ' + result[fields.classes].replace(/"/g, '') + '">' |
: html += '<div '; | ||
|
||
html += result[fields.id] !== undefined | ||
? ' id="' + result[fields.id] + '" ' |
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.
Needs to be sanitzed (double quotes removed
? ' id="' + result[fields.id] + '" ' | |
? ' id="' + result[fields.id].replace(/"/g, '') + '" ' |
: '<div '; | ||
|
||
html += result[fields.id] !== undefined | ||
? ' id="' + result[fields.id] + '" ' |
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.
Needs to be sanitized (double quotes removed)
? ' id="' + result[fields.id] + '" ' | |
? ' id="' + result[fields.id].replace(/"/g, '') + '" ' |
: ''; | ||
|
||
html += result[fields.classes] !== undefined | ||
? ' class="result ' + result[fields.classes] + '">' |
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.
Needs to be sanitized (double quotes removed)
Seeing my comments, i currently disagree (or at least saying it's a breaking change) to the "use a div if no url exists" logic. |
Give much more flexibility on the type of content and theming possibilities for each individual search result.
We could already manipulate the response received by the Search API:
The problem is that we couldn't inject the
id
andclass
attributes to each individual item rendered by the Search module.Now, depending on the status of each response, the status potentially reflecting a given website's logic and needs, we can inject an id and additional classes that can then be used for theming and manipulating.
Also, before each item was wrapped within a
<a>
tag. Because HTML prohibits having nested<a>
tags (which makes sense), we couldn't inject any link into the body of individual items (e.g. inject relevant links to open in a different browser tab, while keeping the result popup menu open).Now, if the result item has
result[fields.url]
, then keep the<a>
tag, because it makes sense: the whole result is assigned an URL.If
result[fields.url]
is absent, the result is wrapped within a<div>
tag, making it possible to inject one or even several links into the result's description, as may be required.This feature is backward-compatible, as the default behavior is unchanged.