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

Register unique clickOutsideToClose click handlers #129

Merged
merged 1 commit into from
Oct 26, 2016

Conversation

oscarni
Copy link
Contributor

@oscarni oscarni commented May 17, 2016

Prevents removal of the listener when multiple dialogs are on the same page.

This resolves an issue where multiple tether-dialogs without overlay and clickOutsideToClose would be available on the same page. Opening one and then directly open another would result in the handler click.ember-modal-dialog being removed and the new opened dialog would be without it.

@lukemelia
Copy link
Contributor

Thanks @oscarni. Any chance we can get a regression test on this?

@oscarni
Copy link
Contributor Author

oscarni commented Oct 19, 2016

Sure, I'll look in to it!

@oscarni
Copy link
Contributor Author

oscarni commented Oct 20, 2016

Have been going crazy over here debugging. It worked flawlessly on ember 2.6, but no longer in 2.7. Apparently elementId is null on tagless components since ember 2.7 emberjs/ember.js#13627

Any tips of how we could create unique event names without elementIds?

@lukemelia
Copy link
Contributor

@oscarini try Ember.guidFor(this)

@oscarni oscarni force-pushed the master branch 2 times, most recently from 64be133 to eb81352 Compare October 21, 2016 09:15
@oscarni
Copy link
Contributor Author

oscarni commented Oct 24, 2016

@lukemelia Ember.guidFor(this) worked like a charm.

I've been working on creating a failing acceptance test as the regression test you proposed, but I can't seem to reproduce the bug in the test environment. The test I crate pass as if there never was anything wrong. But when I click manually in the app I can clearly reproduce the bug 100% of the time. It's like events are handled differently or something in the test environment, so they can't be reproduced the same way as when I manually reproduce the issue.

I've done 2 versions of the test with and without the test helpers in this addon. These are based on master and does not have the fix this PR contains.

master...oscarni:acceptance-test1
and
master...oscarni:acceptance-test2

In a nutshett I've added "another" tethered modal dialog in the Without Overlay - Click Outside to Close section and then click on the buttons respectively to reproduce the issue.

Any suggestions? Otherwise I guess this can't be tested properly. So up to you if you want to merge it or not, but atleast for me it fixes the issue where tethered modals are loosing their event binding and can no longer be closed by outside clicks.

@lukemelia
Copy link
Contributor

@oscarni Let's go with the version 2.

@oscarni
Copy link
Contributor Author

oscarni commented Oct 25, 2016

Unfortunately now tests fail in ember < 2.5.0. Might be due to events fired since 2.5 being more like real behavior emberjs/ember.js#12575.

When running the app in ember 2.4.x and clicking manually in chrome I see the expected behavior, even though tests fail.

Any tip on how to proceed? 🙃

@lukemelia
Copy link
Contributor

@oscarni I'd suggest reviewing what @cibernox did for ember-power-select and testing against older Ember versions since he was responsible for the testing behavior change you linked to.

@cibernox
Copy link

cibernox commented Oct 25, 2016

What I did is create a nativeMousedown and nativeMouseup helper functions that fires always native events in integration tests. You could create a nativeClick helper.

In acceptance, I've created a few utility helpers (available to the users too and documented) that means that pretty much nobody has to manually interact themselves with the component in acceptance tests, which makes sense because acceptance tests should be about testing you app, not that 3rd party addons work.

@oscarni
Copy link
Contributor Author

oscarni commented Oct 26, 2016

Thanks @cibernox for the help! I created a nativeClick acceptance test helper based on your earlier work in ember-power-select. Great stuff!

@lukemelia I created an acceptance test helper file so it'll be easy to add more in the future and extract if you'd like to make them available to users (Great idea, but feels out of scope for this PR though).

I've updated the modal assert helper to use the nativeClick helper and all test pass 👯‍♂️

@lukemelia
Copy link
Contributor

Looks great, @oscarni. Nice work. Can you squash these 3 commits into a single one with a good commit message? After that, I will merge.

- Registers unique click event handlers for each modal to prevent accidental removal of the wrong one.
- Adds acceptance test helper nativeClick to help with testing on ember versions < 2.5.
@oscarni
Copy link
Contributor Author

oscarni commented Oct 26, 2016

@lukemelia Feeling ready, let me know if you have any other comments 🤓

@lukemelia lukemelia merged commit 8a8f262 into yapplabs:master Oct 26, 2016
@patrickberkeley
Copy link

Does this allow for multiple simultaneous modals to each have clickOutsideToClose set to true? If so, is there anything special that's needed? When I have multiple modals on the page and click outside, all of them close.

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.

4 participants