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

Remove imported CHART.JS parts from distribution file #18

Merged
merged 2 commits into from Mar 24, 2022

Conversation

stockiNail
Copy link
Contributor

@stockiNail stockiNail commented Mar 24, 2022

Fix #17

This PR excludes the CHART.JS parts from distribution file of the plugin.
It removes also the direct dependency from @kurkle/color, leveraging on what it's included in CHART.JS.

@stockiNail
Copy link
Contributor Author

@kurkle not clear how to exclude @kurkle/color. Whatever import I set sounds wrong. :(
Maybe it's not possible.

@kurkle
Copy link
Owner

kurkle commented Mar 24, 2022

I think importing color from 'chart.js/helpers' should work, but I guess you already tried that?

@stockiNail
Copy link
Contributor Author

I think importing color from 'chart.js/helpers' should work, but I guess you already tried that?

No I didn't but it works!!! ur awesome!

@sonarcloud
Copy link

sonarcloud bot commented Mar 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kurkle
Copy link
Owner

kurkle commented Mar 24, 2022

I think importing color from 'chart.js/helpers' should work, but I guess you already tried that?

No I didn't but it works!!! ur awesome!

Only doubt is if it works with chart.js v2, I don't remember if it has a compatible export.

@stockiNail
Copy link
Contributor Author

Only doubt is if it works with chart.js v2, I don't remember if it has a compatible export.

Ok, I'll test it against version 2.9.4 before releasing the PR

@stockiNail
Copy link
Contributor Author

It sounds working with 2.9.4 but the issue is not the color part but in version2, the scale object doesn't have parse method.

With #16, the support of CHART.JS 2.x was dropped....

@kurkle
Copy link
Owner

kurkle commented Mar 24, 2022

It sounds working with 2.9.4 but the issue is not the color part but in version2, the scale object doesn't have parse method.

With #16, the support of CHART.JS 2.x was dropped....

Right, that one should be fixed then. Chart.version is a good checkpoint for v3, in case you want to do it :)

@stockiNail
Copy link
Contributor Author

absolutely yes! I'll do in this PR, ok? or do you prefer another one? For me it's the same

@stockiNail
Copy link
Contributor Author

Created another PR #19 for fixing

@stockiNail stockiNail marked this pull request as ready for review March 24, 2022 15:16
@kurkle kurkle merged commit 657e5ea into kurkle:master Mar 24, 2022
@kurkle kurkle added the bug Something isn't working label Mar 24, 2022
@stockiNail stockiNail deleted the rollup branch March 24, 2022 15:20
@stockiNail
Copy link
Contributor Author

I have created additional workload to you.... sorry, thank you for everything!

@kurkle
Copy link
Owner

kurkle commented Mar 24, 2022

I have created additional workload to you.... sorry, thank you for everything!

You also fixed some major issues with this plugin, so thank you! :)
I'm going to push these changes as v0.3.0, and do the docs & samples later when I have the time.

@kurkle
Copy link
Owner

kurkle commented Mar 24, 2022

(would have done this right away, but actions are down again, so will check back later)

@stockiNail
Copy link
Contributor Author

Ok, no hurry, As said, I can go ahead using my local dist file I have to develop some java classes, test them, create doc and so on therefore I need some days. Furthermore we have planned to go to next version in 1 month (otherwise now too close to the previous one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribution file sounds quite big
2 participants