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

Fix click.away regression #2084

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Sep 17, 2021

Quite a few users mentioned that 3.4.4 broke their app when using x-show + click.away.

Annoyingly, the code did pass the test (https://github.com/alpinejs/alpine/blob/main/tests/cypress/integration/directives/x-on.spec.js#L353) but the exact same example loaded on a page is broken (https://codepen.io/SimoTod/pen/OJgQmba) 🤷

I checked the reason for the fix (navigating to different tabs) and setTimeout won't pause like requestAnimationFrame so it should be fine plus is consistent with what we do in x-show (https://github.com/alpinejs/alpine/blob/main/packages/alpinejs/src/directives/x-show.js).

Closes #2080

@calebporzio
Copy link
Collaborator

Thanks @SimoTod - I felt nervous about removing that but all of the tests AND my manual tests passed. I'll try to add a test fo this case. Thanks!

@calebporzio calebporzio merged commit 53b087e into alpinejs:main Sep 18, 2021
@calebporzio
Copy link
Collaborator

Tagged 3.3.5 with this fix and added a manual test for it. Thanks again!

@aaronjpitts
Copy link

@SimoTod FYI this has introduced a bug: #2094

@SimoTod
Copy link
Collaborator Author

SimoTod commented Sep 20, 2021

@aaronjpitts was it working in 3.3.3?
3.3.4 was completely breaking click.away

@aaronjpitts
Copy link

@SimoTod yes, here it is working with 3.3.3: https://codepen.io/aaronjpitts/pen/WNOzBgW

@SimoTod
Copy link
Collaborator Author

SimoTod commented Sep 20, 2021

@aaronjpitts Pretty annoying. All 3 versions have bugs for specific use case. So the problem here is that the nested x-show using a transaction doesn't like the fix (it works okay if a transaction is also defined on the parent x-show. I'll have a look at some point today. In the meantime, you can use a previous version. Thanks for letting me know.

@Tim-Wils
Copy link

Tim-Wils commented Sep 20, 2021

@aaronjpitts @SimoTod
Maybe this could be of help.
It appears the child-elements should also be aware of which function should be called.

Hacked away Aarons 3.3.5 example into a Fiddle and wrapped the "if/else" in the walker function into https://github.com/alpinejs/alpine/blob/main/packages/alpinejs/src/directives/x-transition.js#L134

https://jsfiddle.net/geouh8mr/ (line 1739 in the jsiddle)

Works for me in Firefox and Chrome, but don't have any testing-time left for it to further test it for performance.

@SimoTod
Copy link
Collaborator Author

SimoTod commented Sep 20, 2021

@aaronjpitts @SimoTod
Maybe this could be of help.
It appears the child-elements should also be aware of which function should be called.

Hacked away Aarons 3.3.5 example into a Fiddle and wrapped the "if/else" in the walker function into https://github.com/alpinejs/alpine/blob/main/packages/alpinejs/src/directives/x-transition.js#L134

https://jsfiddle.net/geouh8mr/ (line 1739 in the jsiddle)

Works for me in Firefox and Chrome, but don't have any testing-time left for it to further test it for performance.

Thanks, @Tim-Wils
Unfortunately, I think it would still not work correctly with click away in that case (I've just updated your example to show the issue https://jsfiddle.net/389qv7yn/)

I think we need to either use setTimeout on the transactionIn call or use something with higher priority in the event loop for clickAwayCompatibleShow

@SimoTod SimoTod deleted the bug/click-away-regression branch November 20, 2021 00:28
@SimoTod SimoTod restored the bug/click-away-regression branch November 20, 2021 00:29
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.

problem with x-show on this release 3.3.4
4 participants