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

chore: refactor and optimize core code #833

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ayoub-Mabrouk
Copy link

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

  • Utilized newer JS features for clearer code.
  • Optimized Performance and Streamlined functions for better efficiency.
  • Enhanced Readability: Improved naming for easier understanding.
    These changes improve both performance and code clarity.

@Ayoub-Mabrouk Ayoub-Mabrouk changed the title chore: refactor zoom and pan logic for performance and readability chore: refactor and optimize core code Sep 18, 2024
@trullock
Copy link

trullock commented Oct 1, 2024

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

@Ayoub-Mabrouk
Copy link
Author

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
Copy link

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
Copy link

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
Copy link
Author

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

@trullock
Copy link

trullock commented Nov 4, 2024

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

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

Successfully merging this pull request may close these issues.

3 participants