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.

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

automatically focus in the textbox when you start a search #1426

Merged
merged 10 commits into from
Oct 16, 2024

Conversation

cellio
Copy link
Member

@cellio cellio commented Oct 9, 2024

Fixes #829 by implementing Taeir's suggestion. (Art approved the approach in the comments there.)

@cellio cellio requested a review from a team October 9, 2024 20:44
Oaphi
Oaphi previously requested changes Oct 11, 2024
});

var search = false;
function focusSearch() {
Copy link
Member

@Oaphi Oaphi Oct 11, 2024

Choose a reason for hiding this comment

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

A couple of notes:

  • avoid using var at all costs, const for unchangeable cariables and let for reassignable is a must:
  • the focusSearch function has a "scope bleed" - it should be called something like toggleSearchFocus and accept a single boolean parameter, which you then pass in the handler function. I'll provide example implementation a bit later;
  • avoid using function style of delaring pure functions, arrow syntax has better semantics in this context;

app/views/layouts/_header.html.erb Outdated Show resolved Hide resolved
Copy link
Contributor

@trichoplax trichoplax left a comment

Choose a reason for hiding this comment

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

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.

@trichoplax
Copy link
Contributor

Straight after recommending making this change in Co-Design instead, I've just seen Art's comment on the issue this pull request fixes:

Probably better to do this in QPixel. Makes more sense for an application-specific use case than doing it as part of the design framework.

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.

@ArtOfCode- ArtOfCode- merged commit 2af6d9b into develop Oct 16, 2024
7 checks passed
@ArtOfCode- ArtOfCode- deleted the cellio/829-auto-focus-search branch October 16, 2024 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically focus search bar after clicking search (search-slide)
4 participants