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

fix: use clientWidth/clientHeight instead of getBoundingClientRect #1395

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

evertheylen
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

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

@evertheylen
Copy link
Contributor Author

evertheylen commented Jan 19, 2023

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
Copy link
Contributor Author

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

@evertheylen
Copy link
Contributor Author

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
Copy link
Collaborator

Excellent thanks for your work! :-)

@gionkunz gionkunz self-requested a review January 31, 2023 08:26
@gionkunz gionkunz merged commit 1067900 into chartist-js:main Jan 31, 2023
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.

Wrong size when chart is rendered inside a transform
2 participants