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

feat: introduce theme config and add qunit-theme-ember as option #1166

Merged
merged 10 commits into from
Jun 11, 2024

Conversation

IgnaceMaes
Copy link
Contributor

@IgnaceMaes IgnaceMaes commented Dec 9, 2023

Introduces a theme config option and adds qunit-theme-ember as option.

Before After
preview2 image

In a follow-up PR, this can be made the default. This can first be released as a minor, and switching the default to Ember would be a major.

@IgnaceMaes IgnaceMaes changed the title feat: use qunit-theme-ember feat: introduce theme option and use qunit-theme-ember as default Dec 10, 2023
@acorncom
Copy link

@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 ...

@patricklx
Copy link

Well, it can then default to old qunit-style. Having the option to easily switch to new style?

@ijlee2
Copy link
Member

ijlee2 commented Dec 12, 2023

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.

@IgnaceMaes
Copy link
Contributor Author

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.

@ef4
Copy link
Collaborator

ef4 commented Apr 5, 2024

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 emberjs/rfcs#1017, which seems possibly unnecessary, at least to make this an option.)

@IgnaceMaes
Copy link
Contributor Author

Thanks for clarifying.

Next steps:

  • Split up this PR to make the theme configurable (independent of qunit-theme-ember)
  • PR to make qunit-theme-ember the default

@wagenet
Copy link
Member

wagenet commented Apr 30, 2024

I'm not doubting, but I'm curious what people are doing that would be broken by a theme change.

@IgnaceMaes IgnaceMaes changed the title feat: introduce theme option and use qunit-theme-ember as default feat: introduce theme config and add qunit-theme-ember as option May 9, 2024
@IgnaceMaes
Copy link
Contributor Author

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.

@IgnaceMaes
Copy link
Contributor Author

IgnaceMaes commented May 15, 2024

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.

@NullVoxPopuli NullVoxPopuli merged commit 0f7373f into emberjs:main Jun 11, 2024
16 checks passed
@ijlee2
Copy link
Member

ijlee2 commented Jun 12, 2024

@IgnaceMaes Before making qunit-theme-ember the default in v9, can you ensure that:

  • You don't apply styles globally (e.g. set font-family in <body>), but only to the DOM elements for qunit and ember-qunit using 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.

@Techn1x
Copy link

Techn1x commented Jun 25, 2024

@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/

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.

8 participants