-
Notifications
You must be signed in to change notification settings - Fork 44
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
Popover closing on click in interactive area #422
Comments
Could you provide a failing test? I would like to know if this is the behavior with current version or this has persisted from previous versions. |
I can only demonstrate on version 1.0.0 as Ember Twiddle doesn't want to download the version above, but my project is using version 1.2.3. The problem is identical |
I think this change is fine but since it would change a behavior that others might be relying on, maybe we can make this an optional code path? I can make it either a component level property or a project build option. This way you can get the functionality you want without the need of a breaking change. How does that sound? |
To be honest, I cannot imagine a situation where this "feature" could be deliberately used. That is, there is a conflict between the API method that closes the popover, and the closing by removing the element when you click on it, which is not obvious. That is, as for me, this is still a bug and not a "feature". About the property for a component ... It doesn't occur to me what to call it. That is, in fact, for correct behavior, you will either need to specify 2 properties, for example, The build option for the project as a whole looks more viable, but it seems to me that it would be logical to make the new check option by default, in order to avoid further use of the old "feature". Thus, new users will have a correct check by default, and old users will see the changes in the list of releases and set the required build option, if necessary. |
Since I'm not the original author and only the most active maintainer, I can pray that maybe @kybishop could shed some light on his original thinking in authoring the code. With that said I have to balance what has been considered "expected behavior" for many releases and what has now been brought to my attention as the "right" way to something. Since you are the only person to raise this concern doesn't mean you are wrong, it just means it's isolated (as of now) to your needs. I think the compromising approach here is to set a this a configuration parameter so that at the very minimum you get this into the origin/upstream code and you don't have fork and maintain your own version. If enough people start raising the issue as one they would like to see as the default, I will consider implementing this change and making the appropriate semver adjustments. Does that sound like a fair solution at this time? |
I have nothing against any solution that looks logical. In my project, I have already redefined the behavior by extending the component in the initializer, and this question was created more to highlight the problem, which, perhaps, not many people have encountered, since they do not use complex components inside the popover, or made their own internal edits like me. In any case, I cannot specify how this addon should develop, which does not negate the fact that the current behavior looks like a flaw, as it generates unexpected behavior. If you think it's more logical to make an additional build option for the new behavior for the first time, before @kybishop sheds light on his idea, I have nothing against |
I'm personally a huge fan of semver and releasing often; too many software projects feel like they need something new and shiny with every major version release. I fully support a major version release with a fix. My only request is that it be cross browser compatible (I noticed the comment about firefox). @BwehaaFox I think the use-case you mentioned simply never came up for me, and my more simple uses worked without issue. At this point I'm far enough removed from Ember that the project is "mine" only so much as it is under my name. @tylerturdenpants you're more than welcome to release and handle things as you wish, the same goes for the other collaborators (within reason, of course). |
I do not know what the minimum browser requirements for this addon are, but |
Now, when clicking on an element inside a popover, which, after clicking, should disappear, it causes the popover to be hidden,
For example, when a button is replaced by a loading indicator:
Clicking on the "Load next page" button closes the popover.
It seems to me that this happens due to an incorrect check in
_hideOnClickOut
:Node in
event.target
has already been removed from the document at this stage, so a search in_popperElement
returns false and the popover is closed.An alternative could be a similar check:
It seems to me that such a condition better reflects the essence of this function.
The text was updated successfully, but these errors were encountered: