automatically focus in the textbox when you start a search#1426
automatically focus in the textbox when you start a search#1426ArtOfCode- merged 10 commits intodevelopfrom
Conversation
app/views/layouts/_header.html.erb
Outdated
| }); | ||
|
|
||
| var search = false; | ||
| function focusSearch() { |
There was a problem hiding this comment.
A couple of notes:
- avoid using
varat all costs,constfor unchangeable cariables andletfor reassignable is a must: - the
focusSearchfunction has a "scope bleed" - it should be called something liketoggleSearchFocusand accept a single boolean parameter, which you then pass in the handler function. I'll provide example implementation a bit later; - avoid using
functionstyle of delaring pure functions, arrow syntax has better semantics in this context;
…liminate the JS entirely, but in the meantime let's commit this improvement
…liminate the JS entirely, but in the meantime let's commit this improvement
ArtOfCode-
force-pushed
the
cellio/829-auto-focus-search
branch
from
ebcb474 to
d9d8d84
Compare
trichoplax
left a comment
trichoplax
left a comment
There was a problem hiding this comment.
I recommend this pull request be made to Co-Design instead, in the file js_src/header.ts, either in the method toggle, which starts on line 32, or otherwise in the constructor for the class HeaderSlideToggle, which starts on line 20.
The toggle method is added as an event listener callback to every slider, so adding the code here when it is only used for the Search slider would be slightly wasteful, but likely imperceptibly so.
Instead modifying the constructor would allow adding the existing toggle method to all of the other sliders, and creating a new callback function just for the Search slider, consisting of calling toggle and then focusing the text box. This would avoid adding the unneeded code to the other sliders, and would avoid the decision being made each time a slider is opened, instead deciding just once at page load.
|
Straight after recommending making this change in Co-Design instead, I've just seen Art's comment on the issue this pull request fixes:
This is a good point, and making a special case in the design framework is a little messy. Maybe it's better to keep this in QPixel for now, and avoid changing Co-Design until #1432 is worked on (which is unlikely to be soon as it seems low priority). At that point there would be no special case in Co-Design, as the browser would handle the autofocus automatically with no JS required. |
4 participants
Fixes #829 by implementing Taeir's suggestion. (Art approved the approach in the comments there.)