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

#594 and #589 Fixes navigation issues with non-searchable controls #595

Merged
merged 1 commit into from
Nov 16, 2015

Conversation

MrLeebo
Copy link
Contributor

@MrLeebo MrLeebo commented Nov 13, 2015

Issue: #594 and #589

For non-searchable controls, I replaced the placeholder <div/> with a readonly <input/> that has a tabIndex and onfocus / onblur callbacks. It receives keyboard navigation and closes when you click the mouse outside of the control.

I added autocomplete and type="search" because my LastPass extension kept trying to treat the input as a password for some reason (not sure why this wasn't happening in searchable controls), so there's probably a smarter way to do that.

@JedWatson
Copy link
Owner

Looks good, thanks @MrLeebo!

JedWatson added a commit that referenced this pull request Nov 16, 2015
#594 and #589 Fixes navigation issues with non-searchable controls
@JedWatson JedWatson merged commit 8deadc8 into JedWatson:master Nov 16, 2015
@azaharakis
Copy link

Hi guys this doesn't seem quite right to me?
You'll still have the same problem with multi-selects, as that code is short-circuited here:
https://github.com/JedWatson/react-select/blob/master/src/Select.js#L478 before it gets the opportunity to render the input
It seems we want to render the input regardless of what type of control it is perhaps composing the props within the conditions and spreading them into the component based on the requirements. I'll try submitting a PR to show you what i'm thinking.

@MrLeebo MrLeebo deleted the keyboard-navigation branch November 16, 2015 16:18
@MrLeebo
Copy link
Contributor Author

MrLeebo commented Nov 16, 2015

@azaharakis @JedWatson Wouldn't it be easier to just delete that line?

@MrLeebo
Copy link
Contributor Author

MrLeebo commented Nov 16, 2015

@azaharakis I'm not opposed to what you're suggesting, I just put up a PR to do the simplest fix so in the mean time there isn't a lingering issue with multi controls.

@azaharakis
Copy link

@MrLeebo, I think you're right. I'm new to the codebase, and my initial thought was the final creation of the input props was being done to late, and could reduce duplication by creating the props that get rendered into the input on creation of the control. I'll need to play around a bit more to understand all the options, and if they are required.

@azaharakis
Copy link

Testing this fix, i've notice some different issues occurring now. The input field thats created in these dropdowns doesn't have a variable width and causes its container to have a min-width of around 150px depending on how you have setup your sass/less. It also means that there is a cursor thats visible on mobile devices in these select boxes as well as creating large white spaces as it drops down to the next line if it doesn't fit.

@MrLeebo
Copy link
Contributor Author

MrLeebo commented Nov 17, 2015

Those were non-issues when it was a <div/> but I couldn't get the onFocus and onBlur event handlers to run unless I made it an <input/>, which is weird because you can attach onFocus and onBlur to a div in plain JS no problem, so I'm not sure what the issue is.

@JedWatson
Copy link
Owner

Sorry for the delay - I've been travelling. Just merged @MrLeebo's second PR, I need to look into this more closely but for the moment I think this makes it better.

azaharakis pushed a commit to azaharakis/react-select that referenced this pull request Nov 26, 2015
This essentially reverts the change made here
JedWatson#595 By applying a tabIndex on a
div element allows it to be focused. This was the reason the div was changed to
an input, by putting it back to a div mobile devices display consistent behaviour
when the select box recieves focus. IOS would display a chrome at the bottom
of the page where as chrome wouldn't.
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.

3 participants