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.

Sign up for GitHub

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 improvements #4393

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Search improvements #4393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2156491
* In search, clicking on excerpt text goes to the corresponding ancho…
clydebarrow Oct 28, 2024
c3c204d
Refine JS
clydebarrow Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
ESPHOME_PATH = ../esphome
ESPHOME_REF = 2024.10.2
PAGEFIND_VERSION=1.1.0
PAGEFIND_VERSION=1.1.1
PAGEFIND=pagefind
NET_PAGEFIND=../pagefindbin/pagefind

Expand Down
51 changes: 38 additions & 13 deletions _templates/searchbox.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
if (!inpel)
return;

var mobileWidth = getComputedStyle(document.body).getPropertyValue("--mobile-width-stop");
const mobileWidth = getComputedStyle(document.body).getPropertyValue("--mobile-width-stop");

function isMobile() {
return window.innerWidth <= mobileWidth;
Expand All @@ -74,26 +74,42 @@

const margin = 20;

function getLink(location, anchors, url) {
if (!anchors || !anchors.length)
return null;
// find the closest anchor at or before the current location
const anchor = anchors.sort((a, b) => b.location - a.location).find(a => a.location <= location);
if (anchor) {
return url + "#" + anchor.id;
}
return null;
}

const resultTemplate = (result) => {
let wrapper = new El("li").class("pagefind-modular-list-result");
const wrapper = new El("li").class("pagefind-modular-list-result");

let thumb = new El("div").class("pagefind-modular-list-thumb").addTo(wrapper);
if (result?.meta?.image) {
const thumb = new El("div").class("pagefind-modular-list-thumb").addTo(wrapper);
let image = result?.meta?.image;
if (image) {
if (image.includes("/_images/"))
image = image.substring(image.indexOf("/_images/"));
Comment on lines +92 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance image path handling robustness.

The current image path manipulation could be more robust:

  1. The substring operation assumes "/_images/" exists
  2. No validation of the image path format

Consider this safer approach:

-            let image = result?.meta?.image;
-            if (image) {
-                if (image.includes("/_images/"))
-                    image = image.substring(image.indexOf("/_images/"));
+            let image = result?.meta?.image;
+            if (image) {
+                const imagePathIndex = image.indexOf("/_images/");
+                if (imagePathIndex !== -1) {
+                    image = image.substring(imagePathIndex);
+                }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let image = result?.meta?.image;
if (image) {
if (image.includes("/_images/"))
image = image.substring(image.indexOf("/_images/"));
let image = result?.meta?.image;
if (image) {
const imagePathIndex = image.indexOf("/_images/");
if (imagePathIndex !== -1) {
image = image.substring(imagePathIndex);
}

new El("img").class("pagefind-modular-list-image").attrs({
src: result.meta.image,
src: image,
alt: result.meta.image_alt || result.meta.title
}).addTo(thumb);
}

let inner = new El("div").class("pagefind-modular-list-inner").addTo(wrapper);
let title = new El("p").class("pagefind-modular-list-title").addTo(inner);
const inner = new El("div").class("pagefind-modular-list-inner").addTo(wrapper);
const title = new El("p").class("pagefind-modular-list-title").addTo(inner);
new El("a").class("pagefind-modular-list-link").text(result.meta?.title).attrs({
href: result.meta?.url || result.url
}).addTo(title);

let excerpt = new El("p").class("pagefind-modular-list-excerpt").addTo(inner);
const excerpt = new El("p").class("pagefind-modular-list-excerpt").addTo(inner);
const locations = result.weighted_locations.sort((a, b) => b.weight - a.weight);
const url = getLink(locations[0]?.location, result.anchors, result.url) || result.meta?.url || result.url;
new El("a").class("pagefind-modular-list-link").html(result.excerpt).attrs({
href: result.meta?.url || result.url
href: url
}).addTo(excerpt);

return wrapper.element;
Expand All @@ -102,9 +118,9 @@

function resizeTarget() {
const searchPos = inpel.getBoundingClientRect();
var leftPos;
var topPos;
var maxWidth = 650;
let leftPos;
let topPos;
const maxWidth = 650;
target.style.width = "auto;"
target.style.height = "fit-content";
target.style.maxWidth = maxWidth + "px";
Expand Down Expand Up @@ -171,7 +187,7 @@

const clickCallback = (event) => {
const path = event.composedPath();
if (path.includes(target) || path.includes(inpel))
if (path.includes(inpel))
return;
hideTargets();
};
Expand All @@ -184,5 +200,14 @@
}
});
}
const input_field = document.getElementById("pfmod-input-0");
if (input_field) {
input_field.focus({preventScroll: true});
input_field.addEventListener("blur", () => {
requestAnimationFrame(() => {
input_field.focus({preventScroll: true});
});
})
}
Comment on lines +203 to +211
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve focus management accessibility.

While using requestAnimationFrame is better than setTimeout, the current implementation could be improved for better accessibility and user experience:

  1. Users can't intentionally move focus away
  2. Could interfere with keyboard navigation

Consider this more accessible approach:

         const input_field = document.getElementById("pfmod-input-0");
         if (input_field) {
             input_field.focus({preventScroll: true});
+            let shouldMaintainFocus = true;
+            
+            // Allow users to opt-out of focus lock
+            input_field.addEventListener("keydown", (e) => {
+                if (e.key === "Escape" || e.key === "Tab") {
+                    shouldMaintainFocus = false;
+                }
+            });
+
             input_field.addEventListener("blur", () => {
-                requestAnimationFrame(() => {
-                    input_field.focus({preventScroll: true});
-                });
+                if (shouldMaintainFocus) {
+                    requestAnimationFrame(() => {
+                        input_field.focus({preventScroll: true});
+                    });
+                }
             })
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const input_field = document.getElementById("pfmod-input-0");
if (input_field) {
input_field.focus({preventScroll: true});
input_field.addEventListener("blur", () => {
requestAnimationFrame(() => {
input_field.focus({preventScroll: true});
});
})
}
const input_field = document.getElementById("pfmod-input-0");
if (input_field) {
input_field.focus({preventScroll: true});
let shouldMaintainFocus = true;
// Allow users to opt-out of focus lock
input_field.addEventListener("keydown", (e) => {
if (e.key === "Escape" || e.key === "Tab") {
shouldMaintainFocus = false;
}
});
input_field.addEventListener("blur", () => {
if (shouldMaintainFocus) {
requestAnimationFrame(() => {
input_field.focus({preventScroll: true});
});
}
})
}

});
</script>
2 changes: 1 addition & 1 deletion pagefind.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ exclude_selectors:
- ".toctree-wrapper"
- ".sphinxsidebar"
- ".breadcrumbs"
- "pre"
glob: "{components,cookbook,guides,projects,web-api}/**/*.html"
root_selector: div[role=main]