search

swipestripe / silverstripe-swipestripe

Ecommerce module for SilverStripe
http://swipestripe.com
33 stars 50 forks source link

Make the JavaScript more extendable #36

Open Zauberfisch opened 10 years ago

Zauberfisch commented 10 years ago

Some parts of the Javascript are pretty much unextendable and unchangeable. I am not sure if I am just to stupid to use entwine namespaces, but I have been trying for ages to overwrite the OrderForm submit handler.

the submit handler is bound using this.on('submit', function(e){}) in onmatch of $('.order-form') We should refactor that to use the entwine onsubmit syntax.

icecaster commented 10 years ago

While I agree that the implementation should be consistent and using the chosen way throughout - I'm somewhat having the usual js debate between simple & dirty or complex & extendible. Looking at the code for the order form, this be rewritten in a few lines of procedural jquery, making it easy to understand, without the need for entwine etc. If greater flexibility was needed the file could be blocked and own handlers would be easy enough to create by adapting the code from the files...

What is your opinion on dirty vs extendible entwine?

Zauberfisch commented 10 years ago

I care a great deal about extensibility, especially because I am in need of extensibility right now, and the order form is causing me a lot of pain. Further more I also actually like entwine, I write my own code in entwine too, and since SilverStripe is using it, I think it would only make sense that SwipeStripe does so too.

frankmullenger commented 10 years ago

It's been a while since I have looked at this. Not clear on entwine namespaces but I think you can use more specific selectors in some cases.

I'd like to continue using entwine, changing it out would probably mean altering some of the modifier modules e.g: https://github.com/frankmullenger/silverstripe-swipestripe/blob/2.1/javascript/OrderForm.js#L21

dependency: https://github.com/frankmullenger/silverstripe-swipestripe-flatfeetax/blob/master/javascript/FlatFeeTaxModifierField.js#L4

If you need to replace the javascript completely you could try replacing the OrderForm completely with Object::use_custom_class() and you can effectively replace the Javascript at the same time if that helps.

thomasbnielsen commented 10 years ago

If going with the entwine stuff - please use a lot of comments. That stuff is so geeky and very few frontenders know how it works. Entwine might bring something to the table that is worth a lot, but please remember that for frontenders like me, it complicates things - more stuff to learn.

frankmullenger commented 10 years ago

@NobrainerWeb fair point, given that SilverStripe uses entwine I think it is ok to keep using it personally but agree we want to keep it simple where possible.

Zauberfisch commented 10 years ago

@frankmullenger well, yes, I could overwrite that class or block the file. But if I need to completely block a file or overwrite a php class just to modify a javascript onsubmit behavior, then there is something wrong. especially because entwine is designed to be extendable.

@NobrainerWeb I actually find entwine to be a lot more comprehensible and clear than regular jQuery spaghetti code. But agreed, comments are always good.