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

Add support for detached canvas element #4591

Merged
merged 2 commits into from Aug 1, 2017

Conversation

@simonbrunel
Copy link
Member

simonbrunel commented Jul 31, 2017

Allow to create a chart from a canvas not yet attached to the DOM (detection based on CSS animations described in https://davidwalsh.name/detect-node-insertion). The resize element (IFRAME) is added only when the canvas receives a parent or when style.display changes from none. This change also allows to re-parent the canvas under a different node (the resizer element following). This is a preliminary work for the DIV based resizer.

Fixes #3790
Fixes #4605

Allow to create a chart on a canvas not yet attached to the DOM (detection based on CSS animations described in https://davidwalsh.name/detect-node-insertion). The resize element (IFRAME) is added only when the canvas receives a parent or when `style.display` changes from `none`. This change also allows to re-parent the canvas under a different node (the resizer element following). This is a preliminary work for the DIV based resizer.
@simonbrunel simonbrunel added this to the Version 2.7 milestone Jul 31, 2017
@simonbrunel simonbrunel requested a review from etimberg Jul 31, 2017
});
});

it('should resize the canvas if attached to the DOM after construction', function(done) {

This comment has been minimized.

Copy link
@etimberg

etimberg Jul 31, 2017

Member

[minor] it looks like this test has the same name as the one before

This comment has been minimized.

Copy link
@simonbrunel

simonbrunel Aug 1, 2017

Author Member

Good catch, thanks!

@simonbrunel simonbrunel merged commit f90ee8c into chartjs:master Aug 1, 2017
3 checks passed
3 checks passed
codeclimate Approved by Simon Brunel.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 94.833%
Details
@simonbrunel simonbrunel deleted the simonbrunel:feat/render-event branch Aug 1, 2017
@hach-que

This comment has been minimized.

Copy link

hach-que commented Aug 2, 2017

FYI This broke our isomorphic app on line 274 of platform.dom.js. The issue is that document doesn't exist for document.createElement('style') under Node.js and somehow that is now being called when the module is being loaded.

@simonbrunel

This comment has been minimized.

Copy link
Member Author

simonbrunel commented Aug 2, 2017

Thanks @hach-que for reporting this issue. Not sure what the best way to fix it, ideally, the DOM platform should not be instantiated on Node.js. Are you able to share your app or a minimal use case?

@hach-que

This comment has been minimized.

Copy link

hach-que commented Aug 2, 2017

We can't share the application source code at this time. We're using react-chartjs2 and we're rendering on both the client and the server. This way when the user requests a page, they get the HTML immediately as the Javascript loads in (once the JS is downloaded it then takes over navigation so that everything is client side).

The best way to check for this stuff is to see if window is set with if (window) {. If it's not present in a global scope, you're not running the browser and should avoid using window or document on startup.

@simonbrunel

This comment has been minimized.

Copy link
Member Author

simonbrunel commented Aug 2, 2017

I'm not familiar with isomorphic apps and I don't understand why Chart.js is evaluated server side. Of course checking window will workaround that issue, but I'm wondering if it would not be better to avoid loading platform.dom.js in the case we are not in a compatible environment (ie. with window and document defined).

@hach-que

This comment has been minimized.

Copy link

hach-que commented Aug 2, 2017

Well react-chartjs-2 just renders out the <canvas> tag on the server, but the issue is that the initialize method in module.exports of chart.js is now trying to access window. So I don't think it's that the code is trying to do any runtime stuff with Chart.js on the server, but rather that the initialize method called in src/chart.js is now trying to access document, so the module can't even be loaded server side now (even if we never actually use any of the functionality that hits the DOM on the server).

It's probably reasonable to just not load platform.dom.js though, especially if all DOM related access is encapsulated in that file. Maybe a platform.null.js implementation that does nothing on every call if window / document is not present?

@hach-que

This comment has been minimized.

Copy link

hach-que commented Aug 2, 2017

(The reason we want the <canvas> element rendered on the server is that you only get the benefits of server side rendering if the HTML emitted by the server exactly matches the HTML that would be rendered by React on first load, otherwise React has to recalculate and rewrite the DOM to match)

This was referenced Aug 28, 2017
yofreke added a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
Allow to create a chart on a canvas not yet attached to the DOM (detection based on CSS animations described in https://davidwalsh.name/detect-node-insertion). The resize element (IFRAME) is added only when the canvas receives a parent or when `style.display` changes from `none`. This change also allows to re-parent the canvas under a different node (the resizer element following). This is a preliminary work for the DIV based resizer.
@maria-grigorieva

This comment has been minimized.

Copy link

maria-grigorieva commented Apr 19, 2019

hi, I have problems with dynamically created canvases in 2.8.0 version.
I'm trying to insert charts in DataTable, in "td" element. So, for each "td" I use appendChild() with add_shartJS() procedure. Chart data and options were taken from the documentation. The only difference is adding { responsive: false }.

add_chartJS(title) {
        var canvas = document.createElement("canvas");
        var id = 'chartjs_' + title;
        canvas.id = id;
        var ctx = canvas.getContext("2d");

        var myChart = new Chart(ctx, {
            type: 'bar',
            data: {
                labels: ['Red', 'Blue', 'Yellow', 'Green', 'Purple', 'Orange'],
                datasets: [{
                    label: '# of Votes',
                    data: [12, 19, 3, 5, 2, 3],
                    backgroundColor: [
                        'rgba(255, 99, 132, 0.2)',
                        'rgba(54, 162, 235, 0.2)',
                        'rgba(255, 206, 86, 0.2)',
                        'rgba(75, 192, 192, 0.2)',
                        'rgba(153, 102, 255, 0.2)',
                        'rgba(255, 159, 64, 0.2)'
                    ],
                    borderColor: [
                        'rgba(255, 99, 132, 1)',
                        'rgba(54, 162, 235, 1)',
                        'rgba(255, 206, 86, 1)',
                        'rgba(75, 192, 192, 1)',
                        'rgba(153, 102, 255, 1)',
                        'rgba(255, 159, 64, 1)'
                    ],
                    borderWidth: 1
                }]
            },
            options: {
                scales: {
                    yAxes: [{
                        ticks: {
                            beginAtZero: true
                        }
                    }]
                },
                responsive: false
            }
        });
        return canvas;
    }

As a result, I have empty canvases.
Also, tried to use this development build, just to be sure, but all the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.