igorlino / elevatezoom-plus

Enhanced elevateZoom - A jQuery image zoom plugin
http://igorlino.github.io/elevatezoom-plus/
MIT License
162 stars 77 forks source link

Elevate zoom plus does not work with the Slim version of jQuery 3.1.1 #68

Open bircha opened 7 years ago

bircha commented 7 years ago

Just to let you know (in case you already don't) that Elevate zoom plus does not work with the Slim version of jQuery 3.1.1.

Fox example Bootstrap 4 promotes the use of the Slim version. See https://v4-alpha.getbootstrap.com/getting-started/introduction/

igorlino commented 7 years ago

Thanks, yes, its true. I was actually not aware. I'm unfortunately out of time for the moment but if you create a fix for that, I would happily merge it in and release a new version.

pixelass commented 7 years ago

@bircha this plugin is so old and dirty I recommend not using it. The code is filled with potential bottlenecks and performance hungry unneeded parts. There is an intense use of old jQuery methods which are now deprecated or not recommended (.bind(), .animate(), .fadeIn()).

I think this plugin should in general be flagged as "outdated" so people stop using it in new projects. Since the maintainer doesn't seem to have time to update the plugin a better label would be "deprecated" which would help explaining my clients why this plugin should be removed from existing code.

igorlino commented 7 years ago

@pixelass WOW, you write the most shameful comment since 2 years. coming from a guy who has given zero '0' lines of code to it.

Yes, I don't want to spend my time in new features or cleanup. The project works as expected for my use cases. Its up to the community to provide improvements. This has worked quite well over 2 years, and people have provided couple of good fixes or cleanups.

I don't know what do you mean by performance hungry, but seems to work in quite some projects. I think several of deprecated methods are nowadays simply proxy method that use the newer recommended methods.

Maybe you are biased because you are using newer front libraries like react or angular.

But, whatever, if you have a better plugin that is not 'outdated', and can be used in new projects, and is not framework specific, please enlighten this not constructive discussion.

LOL

pixelass commented 7 years ago

I didn't intend to offend you but the code is outdated. Why are there several slow String() calls when they're not needed. Why are conditionals so deeply nested and don't make use of the 'else'. Why are there no tests and zero documentation. Why are there undefined globals.

It looks like you wrote this plugin in a hurry and never chose to update it.

BTW. I usually write vanilla JavaScript because dependencies suck.

You should take it as a compliment that I spent the time to read all that spaghetti.

igorlino commented 7 years ago

If you read the project description, the plugin was not written by me, but I clone it from elevatezoom. I did quite a lot of changes on my free time, you can see the initial GIT commits. The original elevatezoom project is dead since several years.

Maybe you want to spend 1-2 days for all your comments and contribute.

pixelass commented 7 years ago

I just opened a PR #73 for some basic stuff. If I find more time I can add some things on top once it's accepted. I fixed some minor issues that are related to jshint and jscs. Also the dependencies have been updated to make sure those are secure.

Just to make sure this is not misunderstood: I do not recommend using this library for production unless contributors (anybody reading this) decide to update the code base. Sadly this library is very complex and linters throw tons of warning unless disabled. No offense... it's just a recommendation for developers that are not good enough to evaluate a plugins code base. I used to be that guy and would have appreciated the honesty. For people who love refactoring this must be a paradise.

I am pretty sure that jQuery lite support could be added but I'm not too sure if this plugin will make you happy. IMHO it would be better to write a new plugin with the required features. (I am saying that after spending about 12 hours trying to refactor this repos code base). I'm pretty sure that the main functionality can be written in a few hundred lines (considering dropping some IE versions that are not supported by jQ3)

I know I can sound mean sometimes but I have the best intentions I hope you understand @igorlino

pixelass commented 7 years ago

I did some testing and this plugin works with jQ 3.2.slim. The example page has some issues since

Without these two dependencies the examples.htm should work without issues.

If anybody is interested here's a branch that "basically" works https://github.com/pixelass/elevatezoom-plus/tree/feature/jquery.3.x.slim

(I removed the easing example. The snippets don't work due to incompatibility)

It seems jQuery slim does not support $().animate therefore more issues occur on some examples (i.e. responsive) Sadly the code base makes it hard to evaluate if this can easily be refactored.

@bircha I hope this information helps you.

igorlino commented 7 years ago

super, amazing work. This feels a lot better :+1:

I have worked in quite many JS projects, so I cannot recall everything. I think I fixed some few JSCS warnings at the beginning. Looking at the initial commits, im surprised I did not do much also using JSHint or similar. I think I was first looking not to break anything, while being able to merge all pull requests from the original elevatezoom project.

pixelass commented 7 years ago

I can try to remove the animate stuff and then add an example page with jQuery slim support (in case the OP still needs this feature @bircha ).

@igorlino there is no browser list. Which browsers are considered as "supported"?

bircha commented 7 years ago

Thanks for the info and the effort! I'll give it a try at some point when I have the chance.

igorlino commented 7 years ago

I'm not 100% sure, but a big range of browsers seems to be supported, only thing is IE where I found one description that the original elevatezoom had support for IE7. I personally do not care for older browsers, if someone needs older browser support they can take an older version of the plugin.