search

chartjs / chartjs-plugin-zoom

Zoom and pan plugin for Chart.js
MIT License
598 stars 327 forks source link

chore: refactor and optimize core code #833

Closed Ayoub-Mabrouk closed 2 weeks ago

Ayoub-Mabrouk commented 2 months ago

This update refines the zoom and pan functionalities with cleaner, more modern JavaScript. Key improvements include:

trullock commented 2 months ago

This contains changed behaviour and IMO is not more readable/cleaner

Ayoub-Mabrouk commented 2 months ago

This contains changed behaviour and IMO is not more readable/cleaner

Thank you for your feedback! I appreciate your thoughts on the changes. The update aimed to refine the zoom and pan functionalities and improve performance, but I understand that you feel the readability has not improved.

Could you please share specific examples or areas where you see issues? I’m open to discussing potential adjustments to enhance clarity while maintaining the benefits of the update.

NotTsunami commented 1 month ago

These code changes don't look like they were made by a human. I agree with trullock in that these changes are not more readable.

That aside, this library does not seem well-maintained.

trullock commented 1 month ago

These code changes don't look like they were made by a human. I agree with trullock in that these changes are not more readable.

That aside, this library does not seem well-maintained.

My fork at https://github.com/trullock/chartjs-plugin-zoom which is published on npm as @trullock/chartjs-plugin-zoom, is in a slightly better state, but I offer no warranties, I've simply merged some PRs and fixed bugs I've encountered

Ayoub-Mabrouk commented 1 month ago

Hi @trullock can you please reply to this https://github.com/chartjs/chartjs-plugin-zoom/pull/833#issuecomment-2387185157 it would be more helpful.

trullock commented 4 weeks ago

Hi @trullock can you please reply to this #833 (comment) it would be more helpful.

I'm afraid I don't have time to do this in depth

but at a minimum:

you've made scores of unrelated (and untested) changes in one go, this is bad because it cant esily be tested and will never be merged

youve made changes that make the code less readable. shorter code is not better code.

Youve also made actual changes, the behaviour is different. I cant remember where, it was a month ago

kurkle commented 2 weeks ago

I have to agree with @trullock, shorter code is not better. Destructuring, inline functions etc generate garbage, which can be the bottleneck of performance.

If you can produce a sample that performs better with these changes, then I would be open to review this. Until that, I'll close this.