Porting to CHART.JS 3 (using master) by stockiNail · Pull Request #390 · chartjs/chartjs-plugin-zoom

stockiNail · 2020-10-22T15:20:42Z

fixes #376

stockiNail · 2020-10-22T15:23:16Z

I submit this PR but I don't have enough experience about karma and testing part, therefore it should be review.
The tests should be anyway ready because I changed them to be align with new CHART.JS.
Be aware that I have tested it with dist/master (last commit) due to there are some new updates which are affecting the plugin

benmccann · 2020-11-17T21:33:06Z

@stockiNail it's possible that the tests are failing because you did not update the package.json file. Can you try updating it to use 3.0.0-beta.6 in both peerDependencies and devDependencies?

benmccann · 2020-11-17T21:34:22Z

samples/pan-bar.html

 <head>
 	<title>Bar Chart Pan</title>
-	<script src="https://cdn.jsdelivr.net/npm/chart.js@2.9.1"></script>
+	<!--script src="https://cdn.jsdelivr.net/npm/chart.js@2.9.1"></script-->


you could probably update this to 3.0.0-beta.6 now that it's out?

stockiNail · 2020-11-17T21:41:55Z

I changed only the code because I'm not familiar with javascript and its compiling tools and configuration.
I'm also now working on some other topics.
I do my best to try asap.

stockiNail · 2020-11-19T16:04:16Z

@benmccann I've started working on that

stockiNail · 2020-11-20T16:19:48Z

@benmccann I've started working on that

FYI, only 1 test is failing now: should be called with default options
Let me take more time to fix it.

benmccann · 2020-11-20T16:36:18Z

src/plugin.js

+	scale.options.max = rangeMaxLimiter(zoomOptions, scale.max - maxDelta);
 }

 function zoomTimeScale(scale, zoom, center, zoomOptions) {


you could remove this function now and call zoomNumericalScale where it's called since that's all it does anyway

Same with panTimeScale below

you could remove this function now and call zoomNumericalScale where it's called since that's all it does anyway

done. I'm still having a look but I decided to push anyway the current updates

Same with panTimeScale below

I think you wanted to write panCategoryScale, doesn't it?

What I mean is panTimeScale just calls panNumericalScale directly. You could just delete panTimeScale and call panNumericalScale where panTimeScale is used to avoid the extra method that no longer is really necessary

done, removed also zoomTimeScale

stockiNail · 2020-11-20T19:18:45Z

@benmccann fixed all test cases.
I have also implemented new defaults field of plugin in order to pass the defaults, instead of set the Chart.defaults.plugin.zoom.
I think now should be ok.

stockiNail · 2020-11-20T19:29:34Z

@benmccann let me outline a couple of topics.
My updates had the goal to be able to use the plugin on CHART.JS 3 (in order to test it on my lib) and therefore I didn't apply any change on the logic or structure. It was simply changes on the calls.
Maybe going to a major version (I think it could be a good idea because CHART.JS is going to a major one) you would like to change and improve some logic or syntax of the current implementation.
In this case my help can be very low because I'm not a JS coder.

Other plugins of chartjs organization are now going to Chart.js 3 and I have seen that their building framework (and/or configuration) is changing.
I don't have any experiences on these topic (for JS) therefore I can not help too much if you would like to keep aligned this project with the others,
Apologize.
Anyway I think @kurkle and/or @simonbrunel can help more than me on this topic.
Maybe there is the goal that all plugins/projects of chartjs organization could be managed/compiled in the same way.

benmccann · 2020-11-20T20:08:09Z

Hmm. Seems I am prevented from merging this because Travis is down. I think I will need someone to disable Travis. Then we can turn on Github Actions (#402)

stockiNail · 2020-11-20T20:46:12Z

Hmm. Seems I am prevented from merging this because Travis is down. I think I will need someone to disable Travis. Then we can turn on Github Actions (#402)

I have tried some changes on Travis configuration but it does not work. It looks like you said.

benmccann · 2020-11-20T21:26:50Z

@stockiNail I switched the build to GitHub Actions on master. It's passing on master, but looks to be failing on this PR. If you can get the GitHub check passing then we can merge this PR and ignore the Travis one that's hanging. I'm not sure if you need to rebase against master or if there's more you'll need to do

stockiNail · 2020-11-20T21:37:57Z

@stockiNail I switched the build to GitHub Actions on master. It's passing on master, but looks to be failing on this PR. If you can get the GitHub check passing then we can merge this PR and ignore the Travis one that's hanging. I'm not sure if you need to rebase against master or if there's more you'll need to do

done! and sounds working

stockiNail · 2020-11-20T21:45:02Z

@stockiNail I switched the build to GitHub Actions on master. It's passing on master, but looks to be failing on this PR. If you can get the GitHub check passing then we can merge this PR and ignore the Travis one that's hanging. I'm not sure if you need to rebase against master or if there's more you'll need to do

done! and sounds working

It went wrong but I don't know why because the branch is currently rebased to the master and locally the tests are working.

benmccann · 2020-11-20T21:47:24Z

Maybe it's running a different sequence of commands. You can run git clean -xdf to blow away all files in .gitignore locally and then try npm ci, npm build, npm test

stockiNail · 2020-11-20T23:08:26Z

@benmccann I have seen the following message in GH actions log that I don't have::

20 11 2020 21:37:46.372:WARN [filelist]: Pattern "/home/runner/work/chartjs-plugin-zoom/chartjs-plugin-zoom/node_modules/chart.js/dist/Chart.js" does not match any file.

The issue seems to be related to the path.
I dont' know why there is chartjs-plugin-zoom/chartjs-plugin-zoom. Sounds an error.

I have changed Karma configuration setting the basePath to '..' and in fact the tests went good (see log GHA):

Chrome 86.0.4240.198 (Linux x86_64): Executed 0 of 0 SUCCESS (0.001 secs / 0 secs)
Firefox 82.0 (Ubuntu 0.0.0): Executed 0 of 0 SUCCESS (0.077 secs / 0 secs)
TOTAL: 0 SUCCESS

but it's wrong to set '..'' because some warnings/issues are still there for this path and also locally it does not work with basePath '..', of course.

Therefore I revert the change, going to default basePath (as formerly was).

kurkle · 2020-11-20T23:26:01Z

You need to make the node_modules/chart.js/dist/Chart.js lower case in karma config.

stockiNail · 2020-11-20T23:31:19Z

You need to make the node_modules/chart.js/dist/Chart.js lower case in karma config.

@kurkle as usual, ur awesome!! THANK YOU! I was getting crazy even because I don't know Karma and GHA....

stockiNail · 2020-11-20T23:34:21Z

You need to make the node_modules/chart.js/dist/Chart.js lower case in karma config.

Anyway something sounds strange, because setting basePath to ".." the tests ended correctly... I don't know why.
But now it works.

kurkle · 2020-11-20T23:45:31Z

Anyway something sounds strange, because setting basePath to ".." the tests ended correctly... I don't know why.

I think it just did not find the tests anymore: TOTAL: 0 SUCCESS

stockiNail · 2020-11-20T23:47:57Z

You are right... :(

benmccann · 2020-11-20T23:51:47Z

samples/zoom-time.html

 	<script src="https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.13.0/moment.min.js"></script>
-	<script src="https://cdn.jsdelivr.net/npm/chart.js@2.9.1"></script>
+	<script src="https://cdn.jsdelivr.net/npm/chart.js@3.0.0-beta.6"></script>
+	<script src="https://cdn.jsdelivr.net/npm/chartjs-adapter-moment@0.1.2/dist/chartjs-adapter-moment.min.js"></script>


Can we make this just https://cdn.jsdelivr.net/npm/chartjs-adapter-moment@0.1.2 instead of https://cdn.jsdelivr.net/npm/chartjs-adapter-moment@0.1.2/dist/chartjs-adapter-moment.min.js ?

benmccann · 2020-11-20T23:54:46Z

src/plugin.js

 }

-function panTimeScale(scale, delta, panOptions) {
+function panCategoryScale(scale, delta, panOptions) {


Can you move this back above panNumericalScale? It's difficult to tell what's change since its location in the file has changed

Not possible, due to:

115:1 warning Function 'zoomCategoryScale' has a complexity of 15 complexity 240:2 error 'panNumericalScale' was used before it was defined no-use-before-define 243:1 warning Function 'panNumericalScale' has a complexity of 13 complexity 413:41 warning Function has a complexity of 11 complexity

If you want to use the panNumericScale into panCategoryScale, we must define it before, otherwise we must remove the rule of no-use-before-define.
As you prefer.

Ok. Let's not use panNumericScale in panCategoryScale to minimize the number of changes in this PR

Ok. Let's not use panNumericScale in panCategoryScale to minimize the number of changes in this PR

Done!

benmccann · 2020-11-21T16:27:45Z

thank you so much for all the work on this upgrade!! appreciate your patience working through the issues and comments

stockiNail · 2020-11-21T17:59:20Z

@benmccann I have to thank you for your patience.
You gave me the possibility to learn a lot about JS world and the chance to support better the users of my lib ad well.
I think this should be a starting point for this plugin because some items could be implemented (i.e. type definitions for TS, Javascript classes instead of adding functions to chart object) like other plugins of chartjs org are doing.

benmccann · 2020-11-21T23:09:53Z

Yes, the code for this plugin is a bit outdated and it would be nice to update it to more closely match the setup of the main library and the other plugins

KoalaBear84 · 2020-11-22T12:05:12Z

Great to see it supporting v3. How can I use this? (As it is not yet released as a version).

Because when I put the plugin.js in my project and add it as a script tag I get the following error:
Uncaught SyntaxError: Cannot use import statement outside a module

And when I add type="module" to the script tag it says this:
Uncaught TypeError: Failed to resolve module specifier "chart.js". Relative references must start with either "/", "./", or "../".

I guess I need to add it all separately then. But having too low knowledge of Javascript module things :)

stockiNail · 2020-11-26T09:59:37Z

Great to see it supporting v3. How can I use this? (As it is not yet released as a version).

Because when I put the plugin.js in my project and add it as a script tag I get the following error:
Uncaught SyntaxError: Cannot use import statement outside a module

And when I add type="module" to the script tag it says this:
Uncaught TypeError: Failed to resolve module specifier "chart.js". Relative references must start with either "/", "./", or "../".

I guess I need to add it all separately then. But having too low knowledge of Javascript module things :)

Answered in #376

porting to CHART.JS 3 (using master)

473cac7

Add args argument to beforeUpdate method of the plugin

2142078

benmccann reviewed Nov 17, 2020

View reviewed changes

benmccann reviewed Nov 20, 2020

View reviewed changes

stockiNail added 3 commits November 20, 2020 19:54

changes tests caes, link to chartjs version 3.0.0-beta6

4a6043a

uses defaults of the plugin instead to set Chart.defauls.plugins

b977402

fixed unit test of 'should be called with default options'

2a4fca0

removes Chart.Zoom.defaults because not used anymore

315339b

stockiNail added 4 commits November 20, 2020 21:33

set only master branch to be able to merge

9d94314

disable travis

0a04a34

try with VM instead of container

d0171a0

fallback to origin configuration

fbcda18

benmccann closed this Nov 20, 2020

benmccann reopened this Nov 20, 2020

benmccann closed this Nov 20, 2020

benmccann reopened this Nov 20, 2020

Merge remote-tracking branch 'origin/master' into chartjs3

c2620c7

stockiNail added 3 commits November 20, 2020 23:46

change base path of karma

d43638e

changes basePath of karma config

79dce44

changes the basePath to see if GH action works

0978754

stockiNail added 3 commits November 21, 2020 00:09

Reverts the change, going to default Karma basePath (as formerly was)

d3835b5

sets actions/checkout@v1 instead of v2

47b0881

adds cmds on run to see current folder

b7143e6

changes to lower case Chart.js link into karma config

e006597

benmccann reviewed Nov 20, 2020

View reviewed changes

stockiNail added 6 commits November 21, 2020 01:11

changes moment URL on samples and removes pan/zoomTimeScale functions

483d136

move pan/zoomNumericScale function below pan/zoomCategoryScale one

4bbbc43

removes empty lines where not needed

b277bca

Newline required at end of file but not found

0abae1e

updates README adding the dependency to Chart.js 3.0.0

3534dbf

fixed README missing dot at the end of sentence.

6d87274

benmccann merged commit 49de6c1 into chartjs:master Nov 21, 2020

KoalaBear84 mentioned this pull request Nov 26, 2020

Compatibility with Chart js 3.0 #376

Closed

stockiNail deleted the chartjs3 branch November 26, 2020 10:00

kurkle added the enhancement label Jan 5, 2021

Conversation

stockiNail commented Oct 22, 2020

Uh oh!

stockiNail commented Oct 22, 2020

Uh oh!

benmccann commented Nov 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stockiNail commented Nov 17, 2020

Uh oh!

stockiNail commented Nov 19, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

benmccann commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

benmccann commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

benmccann commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

kurkle commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

kurkle commented Nov 20, 2020

Uh oh!

stockiNail commented Nov 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann commented Nov 21, 2020

Uh oh!

stockiNail commented Nov 21, 2020

Uh oh!

benmccann commented Nov 21, 2020

Uh oh!

KoalaBear84 commented Nov 22, 2020

Uh oh!

stockiNail commented Nov 26, 2020

Uh oh!

Reviewers