Skip to content

Conversation

@herecydev
Copy link

Resolves #1426

@herecydev
Copy link
Author

@ljharb let me know if the PR works for you

@herecydev
Copy link
Author

Going to need some help with this one. I think I can get the suite in a place where the jsdom tests pass. The issue is (as evidenced by the failing builds) is that JSDOM is really not happy with running in a browser (which occurs when the karma tests are run). To that end, I'm after a good strategy to seperate the jsdom tests so that they run exclusively during normal mocha run.

I feel like the easiest way is to have a test/utils/calculateDimension_spec.jsdom.js and filter this out within the karma.conf.js. This isn't ideal but I'm battling against the reality of where jsdom is and the boundaries it can operate in.

Comment on lines -19 to +26
parseFloat(style[`padding${axisStart}`])
+ parseFloat(style[`padding${axisEnd}`])
+ parseFloat(style[`border${axisStart}Width`])
+ parseFloat(style[`border${axisEnd}Width`])
getStyleOrDefault(style, `padding${axisStart}`)
+ getStyleOrDefault(style, `padding${axisEnd}`)
+ getStyleOrDefault(style, `border${axisStart}Width`)
+ getStyleOrDefault(style, `border${axisEnd}Width`)
Copy link
Member

Choose a reason for hiding this comment

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

i think inline || 0's here would be clearer

Comment on lines +76 to +80
describe('withMargin false and borderBox true when style properties are absent', () => {
let testElement = null;

beforeEach(() => {
const { window } = new JSDOM('<div />');
Copy link
Member

Choose a reason for hiding this comment

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

i think maybe it'd make more sense to do one mocha run without jsdom, and another with? then describeIfWindow would be used to skip the DOM tests for that run.

That would also mean the karma tests wouldn't run with jsdom, since it'd be an entirely different test run.

@ljharb ljharb marked this pull request as draft November 30, 2021 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warning: NaN is an invalid value for the height css style property.

2 participants