-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Added isFocused to several components #3448
base: master
Are you sure you want to change the base?
Conversation
@JedWatson Would you mind taking a look at this? |
Also, it looks there are a lot of other PRs that need some attention. Have you thought about adding a maintainer to the repo? I'm less familiar with this library, but I'd be willing to help take a first pass at some of the PRs and issues. |
Would you mind fixing the conflicts? :) I'm assuming it'll be prioritized if all other variables are in place |
Also refactored to reduce a lot of duplication and hopefully avoid this issue with any other common props in the future
Conflicts resolved. @JedWatson or @gwyneplaine can you review this? |
@JedWatson @gwyneplaine bump |
bump |
I'll take a look at this tonight or this weekend. Seems pretty straight forward and will add this as a candidate for next release pending no issues. |
Overall, the code seems sound. I also realize that not having these props makes the Indicators much more difficult to know when to hide/show. I guess the one concern I have is the existing naming as I guess the question is, would the user know, for example, that isFocused refers to the Input and not the custom Menu component? It seems that Option is the single exception of a Component that has these props but they represent something else. I did check that that the Option overrides the values I'll bring this to conversation this week and see what others think. |
Are you saying that I'm not worried about the naming since the distinction between:
is pretty obvious. If it IS that the value propogates, I still don't think it's a big deal. I think it's already a bit confusing that we can check on whether or not the placeholder is focused... It seems that |
@kylemh I think @ebonow is saying it's situations like this where the |
I think ideally we would come up with a different prop name for either the general case or the differing specific cases. Unfortunately that would be a breaking change. |
Understood. As a non-breaking change we could document the confusing behavior of |
Here's the rub. For the default behavior, it is dependent on the input, for when So technically only one thing ever can be focused and that focus determines a lot of things including the onFocus and onBlur callbacks. In fact as somewhat of a tangent, I'd like to consider replacing the dummyInput with some other focusable element to address this: #3526 Since none of the other elements ever receive focus, it's fair to say that isFocused is accurate and correlates specifically to the Select state, but of course that might change. I would say it's probably accurate as is, but again might not be clear, and from this, would we perhaps want to instead address the Option prop/state as maybe
I'm trying to also balance the impact of #4195 as adding a All that to say, yes this would incur breaking changes if we wanted to do any prop name changes, nor do I know what we would want to change or even if we should change. I simply want to err on the side of caution though it is already implied in many areas that this refers to the Select state (with the single exception of Option). |
Just to clarify, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Several components were not receiving the
isFocused
prop, because it had to be passed in individually to each one. I've refactored it so thatisFocused
is part of thecommonProps
object which gets passed into all the right places already. I also movedisDisabled
up into that group, to remove duplication.