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

Drive initial search from URL query string #83

Merged
merged 1 commit into from
Dec 7, 2014
Merged

Drive initial search from URL query string #83

merged 1 commit into from
Dec 7, 2014

Conversation

JamesMGreene
Copy link
Contributor

Fixes #82

search.focus();
var activeEl = document.activeElement;
if (activeEl && search.el.contains(activeEl)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this if necessary?

  • document.activeElement is always truthy
  • search.el.contains(activeEl) should always be true as you're calling search.focus()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this if necessary?

It's a stylistic thing IMHO. If the maintainers dislike it, I'm fine with changing it.

document.activeElement is always truthy

Usually but not always, e.g. document.activeElement can be null, at least in IE while the page is initially loading. You're right in that it should be a non-issue if search.focus() does its job, though.

search.el.contains(activeEl) should always be true as you're calling search.focus()

True based on the application-level view of things but that is dependent on the current function of the various components being pieced together. It may be over-cautious but it certainly isn't invalid.

shrugs

Like I said, I'm fine with changing whatever makes this get merged. 😛

@stephenmathieson
Copy link
Contributor

why was build/build.js removed?

@JamesMGreene
Copy link
Contributor Author

why was build/build.js removed?

It wasn't, GitHub just doesn't want to show the diff because there are too many changes.

https://github.com/JamesMGreene/component.github.io/blob/3bbde7855a99020ed89e501cace4b97650da9425/build/build.js

@stephenmathieson
Copy link
Contributor

haha oops :p

@JamesMGreene
Copy link
Contributor Author

The other approach I had in mind would eliminate all of this document.activeElement stuff and just add a new method to Search instead, e.g.

// In "boot/index.js":
var initialQuery = qs.parse(location.search).q || '';
search.input(initialQuery);
search.show(initialQuery);
// In "search/index.js":
SearchView.prototype.input = function(query) {
  this.el.querySelector('input').value = query;
};

@stephenmathieson
Copy link
Contributor

+1 to SearchView#input

@JamesMGreene
Copy link
Contributor Author

+1 to SearchView#input

OK, will do.

Fixes #82

Also now auto-focuses into the Search box on load.
@JamesMGreene
Copy link
Contributor Author

Amended.

Other new changes worth noting:

  • Now always auto-focuses into the search box on load
  • Initial URL query now calls search.search() instead of search.show(initialQuery) in order to maintain query analytics
    • SearchView#search had to be updated to fallback to getting the query value from the associated input element if it wasn't triggered by a DOM event

@JamesMGreene
Copy link
Contributor Author

Initial URL query now calls search.search() instead of search.show(initialQuery) in order to maintain query analytics

That said, separate issue from this PR: the site's analytics object used within SearchView#search doesn't seem to exist:

image

@timaschew
Copy link
Member

thanks. Have no time this week, but I will check it next week or maybe someone else :)

timaschew added a commit that referenced this pull request Dec 7, 2014
Drive initial search from URL query string
@timaschew timaschew merged commit c225721 into component:master Dec 7, 2014
@JamesMGreene
Copy link
Contributor Author

🤘

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.

Drive search from query string
3 participants