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

Refactor to logical entities #452

Merged
merged 7 commits into from
Apr 16, 2021
Merged

Refactor to logical entities #452

merged 7 commits into from
Apr 16, 2021

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Apr 15, 2021

image

@kurkle kurkle added the chore label Apr 15, 2021
@kurkle kurkle requested review from etimberg and jledentu April 15, 2021 17:19
@kurkle
Copy link
Member Author

kurkle commented Apr 15, 2021

Size Change: -127 B (-1%)

Total Size: 15.4 kB

Filename Size Change
dist/chartjs-plugin-zoom.esm.js 5.83 kB -72 B (-1%)
dist/chartjs-plugin-zoom.js 6.09 kB -76 B (-1%)
dist/chartjs-plugin-zoom.min.js 3.46 kB +21 B (+1%)

@kurkle
Copy link
Member Author

kurkle commented Apr 15, 2021

I'll try something to get a better diff, this is a huge review.

Edit: does not look like I can get git to detect moved functions.

etimberg
etimberg previously approved these changes Apr 15, 2021
Copy link
Collaborator

@jledentu jledentu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome refactoring! 👏

I've found an issue in the zoom-time sample. The chart disappears when dragging, and there are console errors:

Uncaught TypeError: chartInstance.$zoom._options is undefined
    beforeDatasetsDraw http://127.0.0.1:8080/dist/chartjs-plugin-zoom.js:688
    callback http://127.0.0.1:8080/node_modules/chart.js/dist/chart.js:796
    ...

@kurkle
Copy link
Member Author

kurkle commented Apr 16, 2021

Thanks @jledentu, did not go through the samples myself. I had not completed the beforeDatasetsDraw hook refactoring.
Completed that and finalized var => let/const changes.

Copy link
Collaborator

@jledentu jledentu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, congrats

@kurkle kurkle merged commit c5c7d47 into chartjs:master Apr 16, 2021
@kurkle kurkle linked an issue Apr 16, 2021 that may be closed by this pull request
@kurkle kurkle deleted the refactor branch April 26, 2021 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional event handlers on axes called unexpectedly on zoom/pan
3 participants