ember-qunit
ember-qunit copied to clipboard
emberjs
feat: introduce `theme` option and use `qunit-theme-ember` as default
Switches to qunit-theme-ember as the default theme.
| Before | After |
|---|---|
Open questions:
- Should it be released behind a macro option? Either as opt-out or opt-in.
- Updated the PR to introduce a
themeconfig option. The Ember theme is used by default. But with this flag going back to the QUnit default is easy. And as a bonus: loading external themes is even easier now as the default theme can be disabled.
- Updated the PR to introduce a
- Is it considered a breaking change?
@IgnaceMaes great idea, I like it a lot! ❤️ My guess would be this would be considered a breaking change (I'm working with a client right now that would break if this ended up in their app) as they have custom CSS and assumptions about the qunit theme that wouldn't be valid any more. But the idea of a facelift to help Ember look modern is great.
Think you could write up a brief RFC? I could see the default needing to be qunit until Ember 6, with a deprecation notice if you haven't specifically chosen an option. When we switch to Ember 6 we could flip the default to your new theme (and potentially have done the switch in blueprints earlier than that). Or something along those lines ...
Well, it can then default to old qunit-style. Having the option to easily switch to new style?
Having the option to easily switch to new style?
I'd also suggest an opt-in. Another library that may be affected if we change the default style is @percy/ember. At the moment, it could be hard to introduce changes to that repo.
Thanks for the input, everyone!
The way to go forward seems indeed making the theme configurable. If the default is to be changed, it should go through an RFC and only be released in a next major version.
Add this behind an option does not require an RFC. Flipping the option on by default would arguably deserve an RFC.
(I came here because of https://github.com/emberjs/rfcs/pull/1017, which seems possibly unnecessary, at least to make this an option.)
Thanks for clarifying.
Next steps:
- Split up this PR to make the theme configurable (independent of
qunit-theme-ember) - PR to make
qunit-theme-emberthe default
I'm not doubting, but I'm curious what people are doing that would be broken by a theme change.
I'm not doubting, but I'm curious what people are doing that would be broken by a theme change.
People could have added their own custom styling or additions to the test UI, which might "break" because of this. Even if it's just visually.
Regardless, it would seem odd to me if I'd have a project and reinstall packages without a lockfile, a minor version would change the default theme 🤔 So changing the default in a major seems the safest route.
The PR has been updated to include the theme option, but keep the same default one as before. This can be released in a minor version.
Once this is merged and released, I will open a second one to flip the default to qunit-theme-ember to be released as major.
@IgnaceMaes Before making qunit-theme-ember the default in v9, can you ensure that:
- You don't apply styles globally (e.g. set
font-familyin<body>), but only to the DOM elements forqunitandember-qunitusing ID or class selectors. - The CSS variables have a namespace (e.g.
--qunit-theme-ember-color-brand) to avoid name collisions. - Your repo has a test Ember app with Percy (for continuous integration).
I tried out the theme in ember-container-query, and think that Percy failed due to global styles from qunit-theme-ember overriding mine.
@mfeckie given the issues listed above for Percy, we might want to verify how/if Snappy is affected by these theme changes https://www.get-snappy.com/
