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

show search button on ios and close when user hits search #1450

Closed
wants to merge 3 commits into from

Conversation

spoeken
Copy link
Contributor

@spoeken spoeken commented Oct 20, 2016

No description provided.

@algobot
Copy link
Contributor

algobot commented Oct 20, 2016

By analyzing the blame information on this pull request, we identified @vvo, @pixelastic and @bobylito to be potential reviewers

@spoeken
Copy link
Contributor Author

spoeken commented Oct 20, 2016

Sorry. I ran a test now and realize the test needs to be fixed. I guess a new context has to be made since it's no longer wrapped in a div but a form element. Unfortunately I'm not too familiar with writing tests.

@spoeken
Copy link
Contributor Author

spoeken commented Oct 20, 2016

changing line 138 to container = document.createElement('form'); and 145 to const wrapper = container.querySelectorAll('form.ais-search-box')[0]; in search-box-test.jswill make the test pass. But it's not true that it wraps the input in a div.

@bobylito
Copy link
Contributor

Thanks for your contribution @spoeken.

I think that changing the root from div to form is alright but is still an unexpected practice for those doing only desktop web dev :) What do you think @vvo?

@vvo
Copy link
Contributor

vvo commented Oct 21, 2016

Hi @spoeken could you walk us through the context of your pull request in more details? We understood it has a relation with mobile and searchbox but since we are not active on those subjects (yet), we need to understand your issue, what was not working and how what your propose is solving it.

Thanks a lot for contributing

@vvo
Copy link
Contributor

vvo commented Oct 21, 2016

Also if you could provide maybe a reference link speaking about that mobile fix (stackoverflow/blog) that would help us commenting the code

@bobylito
Copy link
Contributor

@vvo there is a discussion on the subject on SO http://stackoverflow.com/questions/4864167/show-search-button-in-iphone-ipad-safari-keyboard
Truth is that this is for fixing a specific behavior on some browsers in specific version.

@spoeken
Copy link
Contributor Author

spoeken commented Oct 21, 2016

The reference @bobylito posted is a good one.
And yes, this one is specifically for ios safari.

The first problem was that the keyboard was showing a return button instead of search. to fix this the input needs to be wrapped in a form that has an action, and the input type must be set to search. Since this is now a form with an action I needed to preventDefault on submit to prevent the form from posting.

The second problem was that when I hit search, the keyboard would not disappear. To fix this I made it so document.activeElement.blur() is triggered on submit.
http://stackoverflow.com/questions/5937339/ipad-safari-make-keyboard-disappear

@vvo
Copy link
Contributor

vvo commented Oct 24, 2016

There's one thing I am worried about if we do this change is that if a user already had a wrapping form then it will do this: form > form > input. Not sure this is an actual issue.

@spoeken Do you think in the meantime you could fix this issue by creating your own searchbox?

@vvo
Copy link
Contributor

vvo commented Oct 26, 2016

@spoeken Looking at it again, if you create a <form> yourself and instantiate the searchBox on a div inside it then you should be able to accomplish what you want here I think. Without the need to break the current implementation.

@spoeken
Copy link
Contributor Author

spoeken commented Oct 27, 2016

@vvo Yes, I thought about that. But I would still need the input type to be set to search.

@vvo
Copy link
Contributor

vvo commented Oct 27, 2016

I think if you target a container which is already an input, this should be ok

@vvo
Copy link
Contributor

vvo commented Nov 1, 2016

@spoeken did you try my proposition to target an already existing input element? Thanks

@spoeken
Copy link
Contributor Author

spoeken commented Nov 7, 2016

@vvo I'll try it out! I have another search box where I wan't to do a redirect on enter, and the solution I proposed here is interfering with it. So I think your suggested approach is better!

@vvo
Copy link
Contributor

vvo commented Nov 8, 2016

Let's close this until we have a better overview/feedback. Thanks

@vvo vvo closed this Nov 8, 2016
@spoeken
Copy link
Contributor Author

spoeken commented Nov 8, 2016

This works!

Markup.
<form action="#" id="ios-hide-keyboard-on-submit" > <input id="search-box" type="search"/> </form>
`
Js to prevent form from submitting and closing keyboard.

var iosForm = document.getElementById('ios-hide-keyboard-on-submit'); iosForm.onsubmit = function(e){ e.preventDefault(); var inputField = document.activeElement; inputField.blur(); };

As it is now, it will trigger blur on desktop as well, which might not be wanted. Thank you for the good feedback and for this great application. Without you this would't be as smooth to make as it was: https://www.vingruppen.no/produkter

@vvo
Copy link
Contributor

vvo commented Nov 8, 2016

😍 Thanks for feedback too

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.

4 participants