GavinJoyce / ember-headlessui

https://gavinjoyce.github.io/ember-headlessui/
Other
92 stars 34 forks source link

fix rootElement when running in /tests #139

Closed mansona closed 2 years ago

mansona commented 2 years ago

We had disabled being able to use http://localhost:4200/tests in our app because a bunch of things were breaking and I tracked it down to the rootElement in <Dialog /> not actually being correct.

After a bit more investigation it turns out that the way that setOwnConfig was being used in config might have been the culprit: https://github.com/embroider-build/embroider/issues/1178

I've swapped that out for ember-get-config because I know it's working in this case and I have tested the dummy app + tests in this repo and our upstream app and it's now working 👍

bertdeblock commented 2 years ago

I feel that ember-get-config is overused, the app's config can be accessed in the component's constructor to determine the portal root.

mansona commented 2 years ago

@bertdeblock yea sure if you'd prefer that I'm happy to switch it over 👍

bertdeblock commented 2 years ago

It's up to the maintainers to make that decision of course, but I just wanted to weigh in on this. Also because, ember-get-config explicitly mentions to only use it when you really don't have access to the container.

mansona commented 2 years ago

@bertdeblock well I think it's also a preference thing. My previous attempt at this PR was conceptually closer to what was there before because it was making use for @embroider/macros (just with a bug).

Also I'm a bit biased in favour of ember-get-config these days because I'm the maintainer of it 😉

I am happy to set it up in the constructor if that gets this PR merged quicker 👍

mansona commented 2 years ago

So I'm not exactly sure what is going wrong here 🤔 The only thing that I can think of is that there is some floating dependency in the ember-try runs that is causing the behaviour to change

I did some investigation and it looks like when you click on the document body or the #ember-testing div (which is under the overlay) it will close all open dialogs instead of just the top one 🤔 The ember-get-config version of this had the same problem with tests too but I couldn't quite figure it out.

Any ideas?

mansona commented 2 years ago

So I tracked down the culprit! https://github.com/GavinJoyce/ember-headlessui/pull/140 is failing even though there have been no breaking changes at all.

It seems like someone has changed the API of click() in ember-test-helpers and that is causing a strange interaction https://github.com/emberjs/ember-test-helpers/pull/1185 . I've reported it here: https://github.com/emberjs/ember-test-helpers/issues/1210

mansona commented 2 years ago

Any chance we could get this merged even though the test suite is blocked by an upstream error? I know it reduces our confidence in the suite but we know that this change doesn't cause the particular error at least 😞

GavinJoyce commented 2 years ago

@mansona would you like commit / publishing access to ember-headless?