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

Skip rendering arrow wrapper when custom arrow renderer returns falsy value #2055

Conversation

mtlewis
Copy link
Contributor

@mtlewis mtlewis commented Oct 12, 2017

Closes #1700.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 92.368% when pulling 93af361 on mtlewis:skip-arrow-zone-when-arrow-renderer-returns-falsy into 7523c20 on JedWatson:master.

@lifejuggler
Copy link

Wouldn't it be better if there is a separate option to disable the arrow functionality like disableArrow: true/false instead of an override like this?

@mtlewis
Copy link
Contributor Author

mtlewis commented Oct 17, 2017

I don’t have strong feelings either way, but my reasoning for doing it this way is that it avoids having two props which have to have an order of precedence. I do agree that a disableArrow prop would make the ‘no arrow’ use case more readable though!

@lifejuggler
Copy link

Same for me just that it would be easier to use the default arrowRenderer when someone wants them, but I am fine with this change... just wish it would get merged ASAP

@mtlewis
Copy link
Contributor Author

mtlewis commented Oct 18, 2017

Just a note that we've worked around this for our use case by adding a rule like the below, which is not as neat as having react-select support skipping arrow rendering completely, but does get the functionality to where we want it today.

.myReactSelect .Select-arrow-zone {
  display: none;
}

@JedWatson
Copy link
Owner

I think this makes sense as a fix for the problem, and is better than adding another prop.

Thanks @mtlewis!

@JedWatson JedWatson merged commit 2024844 into JedWatson:master Oct 19, 2017
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.

None yet

4 participants