chartist-js / chartist

Simple responsive charts
https://chartist.dev
MIT License
13.33k stars 2.54k forks source link

fix: use clientWidth/clientHeight instead of getBoundingClientRect #1395

Closed evertheylen closed 1 year ago

evertheylen commented 1 year ago

Fixes #1065.

(Two tests fail on my PC both with and without my change. I verified one of them in my browser where the test passes.)

evertheylen commented 1 year ago

You can test these changes quickly by monkey patching the Svg class:

import { Svg } from "chartist";
Svg.prototype.width = function () {
  return this._node.clientWidth;
};
Svg.prototype.height = function () {
  return this._node.clientHeight;
};
gionkunz commented 1 year ago

Hi @evertheylen, thanks for the PR! :-)

Can you tell me which tests fail on your local machine? What environment are we talking about?

evertheylen commented 1 year ago

Hi! They are the same tests as in the CI. I did something wrong. The two tests are Charts › BarChart › grids › should draw grid lines and Charts › LineChart › grids › should draw grid lines. Essentially, my change makes it draw a single grid line whereas the test expects 3.

However, what I still think the test environment is at fault here. I created the following test:

      fit('clientWidth vs getBoundingClientRect', async () => {
        await createChart();

        const svgNode = (chart as any).svg._node;
        expect(svgNode.getBoundingClientRect().width).toBeCloseTo(
          svgNode.clientWidth
        );
      });

Which fails:

FAIL  src/charts/BarChart/BarChart.spec.ts
  ● Charts › BarChart › grids › clientWidth vs getBoundingClientRect

    expect(received).toBeCloseTo(expected)

    Expected: 0
    Received: 500

    Expected precision:    2
    Expected difference: < 0.005
    Received difference:   500

       95 |
       96 |         const svgNode = (chart as any).svg._node;
    >  97 |         expect(svgNode.getBoundingClientRect().width).toBeCloseTo(
          |                                                       ^
       98 |           svgNode.clientWidth
       99 |         );
      100 |       });

      at Object.<anonymous> (src/charts/BarChart/BarChart.spec.ts:97:55)
evertheylen commented 1 year ago

I found the problem, clientWidth and clientHeight simply weren't mocked. I added a fix for that.

evertheylen commented 1 year ago

Hey @gionkunz, could you approve the workflow so I know for sure whether I fixed the tests correctly? And I'd also like to hear your thoughts on the change itself :)

gionkunz commented 1 year ago

Excellent thanks for your work! :-)