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

Feat/mobile improvements #1999

Merged
merged 4 commits into from Feb 23, 2017

Conversation

Projects
None yet
4 participants
@Haroenv
Copy link
Member

Haroenv commented Feb 21, 2017

What project are you opening a pull request for?

  • react-instantsearch (use v2 base)

Summary

There are some things that can be better in a searchbox by default. Since I enabled those in yarn search, I should move them upstream too.

cc @Shipow

Result

  1. keyboard dismisses when you search

  2. don’t correct spelling

    this takes up space and is almost never useful in a searchbox

    Google's searchbar and the default Safari search disable autoCorrect, spellCheck and autoComplete too

  3. make “return” show as “search” on iOS Safari

    requires an action to the form. Since this is created and controlled by JS, it’s basically a no-op.

before after
before new
@@ -120,6 +120,7 @@ class SearchBox extends Component {
onSubmit = e => {
e.preventDefault();
e.stopPropagation();
this.input.blur();

This comment has been minimized.

@mthuret

mthuret Feb 22, 2017

Member

Are we sure we want to change permanently this behaviour, removing the focus each time someone submit a search when searchAsYouType is disabled? This can be used even if not on mobile, even if I'm not sure it makes sense. What do you think?

This comment has been minimized.

@bobylito

bobylito Feb 22, 2017

Contributor

It only occurs when you submit so I guess that's ok, no?

This comment has been minimized.

@Haroenv

Haroenv Feb 22, 2017

Author Member

although this is a breaking change in a way, a user expects something to happen when pressing "search". You would need to tab back into the input field, but that's how it is in other search that doesn't do search as you type (amazon for ex)

This comment has been minimized.

@vvo

vvo Feb 22, 2017

Member

Same behaviour on Google and Facebook, submit load the search and focusout. should be safe enough

@mthuret
Copy link
Member

mthuret left a comment

Cool improvements!

Do you know why we need to put action="" to get the button "search" on iOS? Why the "type=search" isn't enough? Although what is the behaviour on android?

Although don't forget to update the snapshots ;)

Haroenv added some commits Feb 21, 2017

feat(searchbox): dismiss keyboard on submit
when pressing "return" the keyboard should disappear
feat(searchbox): don't correct spelling
this takes up space and is almost never useful in a searchbox

Google's searchbar and the default Safari search disable autoCorrect, spellCheck and autoComplete too
feat(searchbox): add action to form
return will show up as "search" in iOS Safari
updated snapshots
they all differ because of the way jest 19 works is different than jest 18 IIRC

@Haroenv Haroenv force-pushed the feat/mobile-improvements branch from f6bbc3a to 6edb7e1 Feb 22, 2017

@Haroenv

This comment has been minimized.

Copy link
Member Author

Haroenv commented Feb 22, 2017

on iOS the action is needed, because it shows as "return" when there's no action -> http://stackoverflow.com/a/26287843/3185307

I couldn't check what the situation is on android because I currently don't have an emulator

@Haroenv

This comment has been minimized.

Copy link
Member Author

Haroenv commented Feb 22, 2017

On Android it looks like this, so autosuggest is not disabled, but the return symbol is a search icon. Of course this is very fragmented, so won't be the same everywhere.

ce009692-f926-11e6-94f5-9e34622c1fef

(sorry for the non-screenshot, it didn't want to upload)

Input type search alone (so this or wasn't needed on android) already shows the icon

@vvo

vvo approved these changes Feb 22, 2017

Copy link
Member

vvo left a comment

LGTM, will rebase in single commit when merging. @mthuret you had pending changes, accept if ok

@vvo vvo added this to Doing (Remove from project when done) in InstantSearch - DEPRECATED (kept for reference, to delete in may) Feb 22, 2017

@Haroenv

This comment has been minimized.

Copy link
Member Author

Haroenv commented Feb 22, 2017

I'm wondering why you would squash three separate improvements. If you prefer it that way, no big deal, but I'd prefer to know why for example the action was added, without the other attributes mattering at that point.

@vvo

This comment has been minimized.

Copy link
Member

vvo commented Feb 22, 2017

I'm wondering why you would squash three separate improvements. If you prefer it that way, no big deal, but I'd prefer to know why for example the action was added, without the other attributes mattering at that point.

The way I deal with it is: are any of those commits self sufficient or are they interdependent and only make sense when all together? If none of the commits are providing a "good enough" state then I prefer to have them all in one and label it as

fix(SearchBox): better mobile behaviour

more explanations here

Because from a CHANGELOG POV it makes more sense than having to read three different commits and infer the ultimate impact on my UI.

Ultimately I would even add a code comment next to action="" because that's a "WTF" moment: there's no way from reading the code to understand why it's here. So adding a comment with a reference to either documentation or a good stackoverflow post is always good.

@Haroenv

This comment has been minimized.

Copy link
Member Author

Haroenv commented Feb 22, 2017

Okay! In this case they all work independently, and that's why I made it in three commits in the first place. This PR will be visible in the blame history though, so it would still be useful. You can make the call 😄

@vvo vvo merged commit ea968b3 into v2 Feb 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@vvo vvo deleted the feat/mobile-improvements branch Feb 23, 2017

@vvo vvo removed this from Doing (Remove from project when done) in InstantSearch - DEPRECATED (kept for reference, to delete in may) Mar 6, 2017

@Haroenv Haroenv referenced this pull request Jul 3, 2017

Merged

docs(RN): fix example #147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment