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

PR 1976 was a breaking change but released as a minor update #2104

Closed
lozjackson opened this issue Apr 22, 2024 · 5 comments · Fixed by #2105
Closed

PR 1976 was a breaking change but released as a minor update #2104

lozjackson opened this issue Apr 22, 2024 · 5 comments · Fixed by #2105
Labels

Comments

@lozjackson
Copy link

lozjackson commented Apr 22, 2024

#1976 was a breaking change for us and should have been a major version update

while updating from 3.0.0 to 3.1.1 a lot of tests broke. After looking into this I found that I had camelCase property keys like this

assert.dom(selector').hasStyle({
  backgroundColor: expectedValue, // this doesn't work in 3.1.x, but worked ok in 3.0.0
});

which worked ok with 3.0.0, but started failing after updating. Turns out that the keys should be hyphenated and cameCase keys don't work any more.

assert.dom(selector').hasStyle({
  'background-color': expectedValue, // This works in both 3.0.0 and 3.1.x
});
@BobrImperator
Copy link
Contributor

Released https://www.npmjs.com/package/qunit-dom/v/3.1.2

Feel free to re-open in case it wasn't fixed 👍

@ijlee2
Copy link
Contributor

ijlee2 commented Apr 23, 2024

@BobrImperator I also noticed the casing issue in ijlee2/embroider-css-modules@bd50d62 and appreciate the quick fix.

Now (or perhaps even before, I'm not sure), qunit-dom@3.1.2 allows both camel and dasherized cases, which may lead to inconsistently written code. Can you perhaps recommend at the moment which one to stick to throughout qunit-dom@v3.x?

@lozjackson
Copy link
Author

@BobrImperator thanks for the quick fix

@BobrImperator
Copy link
Contributor

BobrImperator commented Apr 23, 2024

@ijlee2 It appears to me that both syntaxes should be supported and people should be able to use the ones that they see fit.

To me personally it'd make more sense to stick to kebab-case even though it's less ergonomic since that's the convention and also how styles must (AFAIK) be set via css.

However the Javascript API allows you to use both camelCase and kebab-case to set and get style properties.
I also don't know whether there are any caveats to kebab-case but if we were to recommend and enforce camelCase then something like variables --myVariable would suddenly become something that needs special treatement 🤷

const element = document.querySelector("a");
const computedStyles = window.getComputedStyle(element);

computedStyle["background-color"] // "rgba(0, 0, 0, 0)"
computedStyle["backgroundColor"] // "rgba(0, 0, 0, 0)"

element.style["backgroundColor"] = "red";

computedStyle["background-color"] // "red"
computedStyle["backgroundColor"] // "red"

Considering the above, it should be decided on project-to-project basis.
qunit-dom in my opinion shouldn't enforce either style and should support both meaning that bugs like this one need to be considered a regression.

@ijlee2
Copy link
Contributor

ijlee2 commented Apr 23, 2024

@BobrImperator Sounds good, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants