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

Bring back disableContainerStyles #1102

Merged

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Jun 27, 2023

Partially resolves: #1100
Blocked by: ember-cli/ember-cli#10296 (locally, I symlinked to this branch)

This this PR is 99% boilerplate (new app), you'll want to review by-commit.
The new app is added in one commit, and all the other commits are actually meaningful.

ember-cli-build.js's ember-qunit#disableContainerStyles was previously undocumented, but in the interest of keeping the behavior for the upcoming v8 release, this PR re-implements the functionality that was temporarily lost in the v2-addon conversion (because v2 addons don't / can't modify the build).

This approach uses @embroider/macros for statically analyzable config (note that @embroider/macros can be used in non-embroider apps).

Additionally, this behavior is now documented in the README with instructions for setup.

Additionally additionally, I switched the way the test-apps reference ember-qunit to use dependenciesMeta.*.injected, which treats the files from ember-qunit as they would exist as installed from npm -- this gives us the highest confidence that how we test is how users of ember-qunit will experience the library.

In order to test this behavior, I added another test-app, which only has one test -- asserting that the container styles are defaults (and different from what our container CSS does)

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review June 27, 2023 18:37
@NullVoxPopuli NullVoxPopuli force-pushed the bring-back-disableContainerStyles branch from 0829d06 to a8b8509 Compare August 9, 2023 00:53
*
* removes the CSS for the test-container (where the app and components are rendered to)
*/
disableContainerStyles: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously an undocumented feature

return window.getComputedStyle(element);
}

test('the styles are present', async function (assert) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are how I test that the styles are included

return window.getComputedStyle(element);
}

test('the styles are not present', async function (assert) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are how I test that the styles are not included

@wagenet wagenet merged commit 4382af3 into emberjs:master Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Beta release issues
3 participants