Restored search hotkey for desktop. #2355
Conversation
sarahboyce
force-pushed
the
fix-search-hot-key-desktop
branch
2 times, most recently
from
549b303 to
9d9dbf6
Compare
adamzap
left a comment
adamzap
left a comment
There was a problem hiding this comment.
I'm still catching up on recent PRs...it looks like we have two elements with the same ID on the page now. I wonder if there's a way to achieve the same behavior with a single element and responsive styling. Do you or others think this is worth pursuing?
| // Update search input placeholder text based on the user's operating system | ||
| (function () { | ||
| const el = document.getElementById('id_q'); | ||
| const el = querySelectorVisible('#id_q'); |
There was a problem hiding this comment.
Would you be opposed to updating the placeholder on both elements?
It would simplify the code here and allow us to factor the querySelectorVisible function that you created out into the other handler. Before this commit, this file didn't add anything to the global namespace.
There was a problem hiding this comment.
I feel like its usually a good idea to use a class or some other selector and not id, if we expect multiple elements to have that id. Or maybe have id like id_q_mobile and id_q for mobile and desktop respectively. Multiple elements having same id is usually considered invalid HTML. I do like the querySelectorVisible function though, and I don't have a super strong opinion.
There was a problem hiding this comment.
This was a bit fiddly to pull off but I think I've managed it. I pushed up a second commit in case the previous version is preferred but now have unique html ids 👍
There was a problem hiding this comment.
It is a bit fiddly. Not to be a pain but I wonder if should have just used a class instead of an id in the first commit and just gone with that. Do you think that would be cleaner and maybe makes more sense?
There was a problem hiding this comment.
If I remove the id, I think I need to remove the label as this is linked to the field using the field id 🤔
Is that acceptable?
There was a problem hiding this comment.
Yeah you are right. That's not acceptable as well. In that case, I think the fiddly bit maybe makes sense and is worth the effort. Thanks!
I originally tried doing this but found it challenging. The search bar duplication aligns with the light/dark mode toggle duplication and show/hide behavior, aligning to that was easier to acheive given the existing html structure |
sarahboyce
force-pushed
the
fix-search-hot-key-desktop
branch
from
9d9dbf6 to
f15d207
Compare
sarahboyce
force-pushed
the
fix-search-hot-key-desktop
branch
from
f15d207 to
c37f9f9
Compare
sarahboyce
force-pushed
the
fix-search-hot-key-desktop
branch
from
7e511ca to
b458667
Compare
| ]; | ||
|
|
||
| const el = inputs.find(isVisible); | ||
| if (!el) return; |
There was a problem hiding this comment.
checkVisibility is baseline 2024, so maybe we can replace the isVisible definition and call with:
const el = inputs.find((el) => el.checkVisibility());Or is that too modern? This section of the README seems outdated. I'll follow up.
There was a problem hiding this comment.
I think we should use it for both updating the placeholder and adding the hotkey, then if it's not supported, it won't be advertised in the placeholder
There was a problem hiding this comment.
I think either way is fine. @sarahboyce are you thinking of using checkVisibility? Apart from that, I think this PR is ready to be merged.
There was a problem hiding this comment.
I have pushed up a commit with this change 👍
3 participants
Fixes #2349
I also added playwright to have a way to test JavaScript (might be controversial and can remove if wanted)