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

FillStyle does not work with Chart.js 2.8.0 #1

Closed
stockiNail opened this issue Mar 18, 2019 · 16 comments
Closed

FillStyle does not work with Chart.js 2.8.0 #1

stockiNail opened this issue Mar 18, 2019 · 16 comments
Assignees

Comments

@stockiNail
Copy link

I'm testing Rough plugin with CHART.JS 2.8.0 and I see that fillStyle does not work.

The legend is changing correctly but the fill of dataset not.

I'm using your sample https://nagix.github.io/chartjs-plugin-rough/samples/interactions.html and changing only CHART.JS, you can see that it does not work.

<head>
	<script src="https://cdn.jsdelivr.net/npm/chart.js@2.8.0/dist/Chart.bundle.min.js"></script>
	<script src="https://cdnjs.cloudflare.com/ajax/libs/rough.js/3.0.0/rough.js"></script>
	<script src="https://github.com/nagix/chartjs-plugin-rough/releases/download/v0.1.0/chartjs-plugin-rough.min.js"></script>
</head>
@stockiNail stockiNail changed the title FiilStyle does not work with Chart.js 2.8.0 FillStyle does not work with Chart.js 2.8.0 Mar 18, 2019
@nagix
Copy link
Owner

nagix commented Mar 26, 2019

Thanks for reporting this. I’m planning to release a new version that supports Chart.js 2.8 in a few days.

@nagix nagix self-assigned this Mar 26, 2019
@nagix
Copy link
Owner

nagix commented Mar 29, 2019

@stockiNail What browser version are you using? This plugin fills arcs and line areas using SVG paths, and rough.js is using 2DPath to draw them, but IE and Edge doesn't support it (see this comment).

@stockiNail
Copy link
Author

@nagix tested it with FF 66.0.2 (64-bit) and Chrome Version 73.0.3683.86 (Official Build) (64-bit). No IE or Edge

@stockiNail
Copy link
Author

@nagix another topic (not related to this project). For my project, I have rewritten the "colorSchemes" public in JAVA, changing on my implementation some logic and features. Oki for you?

@nagix
Copy link
Owner

nagix commented Mar 29, 2019

@stockNail Hmm, did it work with Chart.js 2.7.3? I’m not able to reproduce the issue in my environment...

Regarding the colorschemes, feel free to modify and distribute it🙂

@stockiNail
Copy link
Author

@nagix importing 2.7.3

<script src="https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.3/Chart.min.js"></script>

it works. See here using "cross-hatch" :

cross-r

importing 2.8.0

<script src="https://cdn.jsdelivr.net/npm/chart.js@2.8.0/dist/Chart.min.js"></script>

it does not work. See here using "cross-hatch" :

cross-w

It does not work also for other fillStyle. It seems it uses ONLY hachure.

Have a look the fillStyle is used into LEGEND.

@nagix
Copy link
Owner

nagix commented Mar 29, 2019

Ok, I understand the problem. It's already fixed in the master, and the fix will be included in the version 0.2.0 that will be released soon.

@stockiNail
Copy link
Author

Thank you very much.

I have seen that if you go to sample "Interactions" and select "dots", it seems it's going in loop and the browser (FF 66.0.2 (64-bit)) shows the message to stop it.

hangdots

@nagix
Copy link
Owner

nagix commented Mar 30, 2019

Rendering a SVG path shape in the 'dots' fill style is very slow, and it seems that a browser is not able to respond due to many requests for animation in a rendering queue. I'm thinking of disabling animation when the 'dots' fill style is selected.

@stockiNail
Copy link
Author

Sounds good. Hopefully the difference of rendering from 'dots' (disabling animation) and all other fill styles will not be so visible to the user.

@nagix
Copy link
Owner

nagix commented Mar 31, 2019

The interaction sample was updated in 3a65a28. I didn't disable animations but set a lower limit of hachureGap based on fillStyle and the chart size. Try this changing fillStyle and the window size.

@stockiNail
Copy link
Author

works perfectly! Thank you. I'm gonna included it in my project asap.

@stockiNail
Copy link
Author

I've checked in deep but the fix for dots is apply only on the sample. Maybe it could be better to add directly into the plugin.

@stockiNail
Copy link
Author

In my opinion, we shouldn't force the user to add a specific code if we know that there is an issue without that code.

Therefore I would suggest to change helpers.rough.js the method getFillOptions as following;

getFillOptions: function(options, canvas) {
	var values = {
		stroke: 'transparent',
		fill: options.backgroundColor
	};
	
	if (options.fillStyle === 'dots' || options.fillStyle === 'dashed' || options.fillStyle === 'zigzag-line') {
		hachureGapMin = Math.ceil(canvas.width * canvas.height / (options.fillStyle === 'dots' ? 20000 : 60000));
		options.hachureGap = Math.max(options.hachureGap, hachureGapMin);
	}

	this.roughKeys.forEach(function(key) {
		if (options[key] !== undefined) {
			values[key] = options[key];
		}
	});
	return values;
},

Accordingly we should also update the caller.

I'm not a javascript expert that's why I didn't create a PR

@stockiNail stockiNail reopened this Apr 1, 2019
@nagix
Copy link
Owner

nagix commented Apr 1, 2019

The reason why I didn't change the plugin code was because the rendering performance varies by browsers, devices and platforms, so there is no reliable threshold for every use case. Also, rendering time changes depending on how many elements are on the chart, element types and the number of data points, so it should be decided by the application code.

I think it is a good idea to mention that this potentially causes a performance issue and how to workaround using hachureGapMin in the README.

@stockiNail
Copy link
Author

Understood. Let me say that from my experience who is using a component like a plugin does not want to take care about this kind of issues or special configurations. Nevertheless if there is not other option, I think it could be a good idea document it, as you wrote

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

No branches or pull requests

2 participants