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 rootElement when running in /tests #139

Merged
merged 1 commit into from
May 4, 2022

Conversation

mansona
Copy link
Collaborator

@mansona mansona commented Apr 6, 2022

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: embroider-build/embroider#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
Copy link
Contributor

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
Copy link
Collaborator Author

mansona commented Apr 6, 2022

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

@bertdeblock
Copy link
Contributor

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
Copy link
Collaborator Author

mansona commented Apr 6, 2022

@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
Copy link
Collaborator Author

mansona commented Apr 6, 2022

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
Copy link
Collaborator Author

mansona commented Apr 6, 2022

So I tracked down the culprit! #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 emberjs/ember-test-helpers#1185 . I've reported it here: emberjs/ember-test-helpers#1210

@mansona
Copy link
Collaborator Author

mansona commented May 3, 2022

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
Copy link
Owner

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

@mansona mansona merged commit eaf4721 into GavinJoyce:master May 4, 2022
@mansona mansona deleted the fix-root-element branch May 4, 2022 14:33
@mansona mansona added the bug Something isn't working label May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants