silvershop / silvershop-core

SilverShop is an e-commerce shopping cart module for the SilverStripe CMS
http://silvershop.github.io
BSD 2-Clause "Simplified" License
113 stars 119 forks source link

Javascript for shop module(s) #232

Closed jedateach closed 9 years ago

jedateach commented 10 years ago

Should we create a separate submodule for shop javascript to be maintained in?

I've always held the position that the module shouldn't rely on javascript. That is, it should at least have fallbacks to full page refreshes etc. I also don't want people to be locked into using a particular set of javascript. This is something I like about SilverStripe itself.

However, I know that a javascript laden e-commerce experience will make it much quicker to use. Our lives would be a bit easier if this javascript lived somewhere where we can all contribute to, and collaborate on it.

To get a javascript/ajax rich ui working nicely, we're probably going to need to make some core code changes. I've discussed this a little with @wilr , where we thought a possible solution could be re-writing the SS_HTTPResponse objects via decorator callbacks so they contain some json, rather than redirect headers.

I would even be open to someone else heading up this effort, and I can offer any assistance & opinions required.

nimeso commented 10 years ago

Maybe if we first make a list of all the Javascript/Ajax features in order of importance?

wilr commented 10 years ago

Progressive enhancement would be good. All links should be normal HTTP requests but I believe javascript should be part of this module, separating it out to another one just seems like a mess of coupling (2x PR's etc). I would favour a single module, javascript could be a flag.

nimeso commented 10 years ago

and yes... agree with separate module :)

On Tue, Feb 18, 2014 at 5:55 PM, Will Rossiter notifications@github.comwrote:

Progressive enhancement would be good. All links should be normal HTTP requests but I believe javascript should be part of this module, separating it out to another one just seems like a mess of coupling (2x PR's etc). I would favour a single module, javascript could be a flag.

Reply to this email directly or view it on GitHubhttps://github.com/burnbright/silverstripe-shop/issues/232#issuecomment-35352589 .

markguinn commented 10 years ago

Totally agree that this would be good. I think the place for this would either be as part of the base bootstrap shop theme or as @wilr said bundled with this module. I prefer the latter. I also think it should be modular, perhaps in the same way that Foundation's javascript is all grouped into separate jquery plugins.

I've never settled on an approach that I really like for ajax on large-scale sites like this. For shop-search I used a namespaced approach more like ExtJS or dojo (ShopSearch.XXX, ShopSearch.Suggest.XXX, etc), but I'm not sure I like it to be honest. :) A more "silverstripe-y" approach may be to use entwine and just keep each feature in it's own file with as much separation of concern as possible. That would allow submodules lie shop-search to plug in to the same architecture.

Just my 2 cents. We may have another big shop project in the next couple months. If that flies I'd be looking to contribute in this area. If not I probably won't have a ton of extra time to help but I'll pitch in where I can.

markguinn commented 10 years ago

@jedateach it looks like the project I spoke of is a go, so myself and @tylerkidd will hopefully be contributing more to the shop eco-system over the next 3-4 months. I'd be keen to focus some energy on this area of the module if you want. Have you thought anymore about this and what approach you'd favour?

jedateach commented 10 years ago

@markguinn Thats great you have some paid time to put towards it.

I'm ok with the javascript sitting in the module itself. It makes sense to maintain them along side each other. Plus, we'll also have javascript for each submodule potentially. Separate modules would just about double the number of shop modules.

So as I see it, the big issues to solve are:

These issues are somewhat linked also.

I say go ahead with what approach you think will be best. You may need to do more research and discussion. I always try to get feedback on ideas before cutting code. I would also try to share early prototypes to get feedback.

@markguinn I'll let you take the lead on the from here on this javascript work. I'd say using this ,or other, github issues is the best way to collaborate. @wilr and I will be very interested in what you come up with, because we'll be needing this work for a project we're currently working on. @wilr how does that sound to you?

wilr commented 10 years ago

Good to hear my dev from @markguinn and of course we've got a bunch to contribute next 2-3 months.

My suggestion would be to utilizie entwine.js for the front end Javascript so developers can 'extend' core click behaviour (say to add modals.. update parts of the UI, etc) easier without replacing large parts of the javascript. Will add ~50kbs onto page load but will provide much more structure. Plus JS can be opt-in, all links should fallback to success messages and redirects.

We're doing things like sliding up a fixed footer with the current order when an item is added to the cart, I wouldn't expect the module to do this but at least I should be able to reuse the existing javascript the shop provides.

markguinn commented 10 years ago

Sounds like we're thinking in the same direction, guys. Here's a link to a document I've been using to whiteboard my ideas. Feel free to add/edit/comment there. Once I settle on a spec/standards/plan I'll post here for review and comment.

https://docs.google.com/document/d/1YGcDAZIBG2mH_kDX5Q5wa6aWl7KpIl_f9P8tflR_21U/edit?usp=sharing

The bit I'm least settled on is how to handle the controller responses transparently. I like your idea of using an extension on Controller to at least add baseline functionality for free. @wilr and @jedateach, you mentioned you had discussed that aspect. Is there any more you could share about that conversation that might be helpful?

markguinn commented 10 years ago

Here's what I'm thinking. Obviously, I'm focused larger than just ajaxifying some buttons here. I'm trying to build a foundation for growth and for submodules to buy into. @jedateach are you good with the coding conventions outlined in the last section?

Requirements:

Big Problems to Solve:

Conventions:

markguinn commented 10 years ago

Another question that comes up here is which theme to test against? Right now, finding a baseline theme is a bit of a mess. Do you have a plan for that Jeremy? Anselm's cloudy theme has pretty good shop support (would need some tweaking) or your bootstrap_shop could be updated to 3.1 and composer-ized, or we could get the simple theme working with shop (would take a lot of work I think).

jedateach commented 10 years ago

Its hard to have the shop look good out of the box without including CSS that will probably need to be replaced when it comes time to implement your own designs. I have tried to keep shop styling next to nothing. This is why I think something like simple, bootstrap, or even a completely unique shop theme would be a good approach.

On 13/03/2014 11:09 am, "Mark Guinn" notifications@github.com wrote:

Another question that comes up here is which theme to test against? Right now, finding a baseline theme is a bit of a mess. Do you have a plan for that Jeremy? Anselm's cloudy theme has pretty good shop support (would need some tweaking) or your bootstrap_shop could be updated to 3.1 and composer-ized, or we could get the simple theme working with shop (would take a lot of work I think).

Reply to this email directly or view it on GitHub.

jedateach commented 10 years ago

..and yes, the SS conventions are probably best I'm guessing. On 13/03/2014 5:10 am, "Mark Guinn" notifications@github.com wrote:

Here's what I'm thinking. Obviously, I'm focused larger than just ajaxifying some buttons here. I'm trying to build a foundation for growth and for submodules to buy into. @jedateach https://github.com/jedateachare you good with the coding conventions outlined in the last section?

Requirements:

  • Progressive enhancement is a must (i.e. everything should work with or without js)
  • Well documented
  • Following wider SS guidelines here: http://doc.silverstripe.com/framework/en/topics/javascript and http://doc.silverstripe.org/framework/en/trunk/misc/coding-conventions
  • Should be easy (ideally automatic) to make the output of a controller action consumable
  • Server can trigger various behaviours in response to a request

    • open new modal (e.g. requiring login to add to wishlist)
    • close modal (e.g. after registration, login, add to cart)
    • redirect page (e.g. to a landing page after ajax login)
    • replace multiple page elements (e.g. sidecart, buttons)
    • trigger jquery events on the document (or some other pub/sub system)
    - technically, this could cover much of the above (e.g.
    open/close modal, redirect)
    - does entwine have a better option than firing jquery events on
    document.body?

Big Problems to Solve:

  • Code organisation
    • best option: jQuery + Entwine + module pattern
    • Controller output
    • Extension on Controller provides baseline (e.g. if ajax, return either a standard "ok" response or the main block of content html, probably the former)
    • Individual actions can override the default if they return json
    • Provide easy methods for success and failure responses
    • Possibly provide a hook to modify the baseline (i.e. onBeforeAjaxResponse)
    • Flexible, re-usable request/response options
    • json response can replace any part of page and trigger any event on document.body
    • a few built-in [overridable] behaviours

Conventions:

  • Organize modules according to features (e.g. "add to cart button" over "product detail page")
  • Entwine should use the 'shop' namespace (see https://github.com/hafriedlander/jquery.entwine/tree/master#namespaces) or 'shop.xxxxx' for modules that want to
    • CSS classes can be used to specify behaviours (i.e. "addToCart" class on a button) - this fits the html5 spec which says that class is used to add semantic information, rather than strictly for presentation.
  • Loading indication should be handled via CSS. I would think it should add a class to the target button/link and also the body tag and remove both when loading is complete
  • Should use the HTTP status code for error responses (see 400, 409, 422, 500)
  • Should generally return json (but be smart enough to handle html if its mistakenly given)
  • Anything beyond jquery, like a modal dialog if needed, should be easily interchangeable (i.e. you can tell it to use jqueryUI, foundation, bootstrap, fancybox, or write your own adapter for any modal plugin)

Reply to this email directly or view it on GitHubhttps://github.com/burnbright/silverstripe-shop/issues/232#issuecomment-37525571 .

markguinn commented 10 years ago

What do you all think of this syntax for ajax-aware controllers:

function add($request) {
    // add product to cart
    return $this->whenAjax()
        ->triggerEvent(‘cartchange.shop’, array(‘action’ => ‘add’, item => ‘blah’, qty => 1))
        ->pushRegion(‘SideCart’)
    ->otherwise()->redirectBack();
    ->buildResponse();
}

I'm not sure I love those method names, but something like that makes the logic very clear. An alternative would be:

function add($request) {
    // add product to cart 
    $this->ifAjax()->triggerEvent(‘cartchange.shop’, array(‘action’ => ‘add’, item => ‘blah’, qty => 1));
    $this->ifAjax()->pushRegion(‘SideCart’);
    return $this->redirectIfNotAjax();
}

Thoughts?

jedateach commented 10 years ago

My initial reaction is that the chained logic syntax seems a little foreign. I've seen unclecheese use it in his bootstrap forms module. I'm not sure if SS has a clearly defined way of handling javascript, but I'd hope that whatever we come up with will be easy (and hopefully familiar) to other developers. Maybe the result of this work will help to define some guidelines for SS ajax development.

The second example looks like it could have two lines wrapped in a single if($request->isAjax()){ block.

I've heard an event system talked about for SilverStripe, but at the moment we only have extension hooks. How do you imagine the triggerEvent stuff working? Here's roughly how I imagined things working with what @wilr and I discussed:

//in ShoppingCart_Controller class
function add($request){
  //do stuff to add to cart (as per current implementation)...
  ShoppingCart::get()->add("....");

$this->extend('onAddResponse', $response, $request, $success);
 return $response;
}

//in ShoppingCartAjax class
function onAddResponse($response, $request, $success){
if($request->isAjax()){
//get stuff, based on request & success of add action
$response->setBody("json/html fragment here");
}
}

Is this meant to update the ShoppingCart_Controller add function? or is this in a different controller? Should there be individual & separate controller actions for ajax? My gut feeling is to put ajax specific code into a separate file/controller dedicated to ajax, then it can be easily contained / maintained. I know that this would mean abstracting some logic out of controllers and into service classes, but thats the direction I'd like to (continue to) take with the shop module. I believe this is also direction SS is moving in, especially now that we can leverage the power of composer. This means that different controllers can perform the same, or similar actions. Ingo's talk on Pagetypeitis somewhat helps to explain this stuff.


I have lots of random additional thoughts, but little idea on what is best, and apologies if I'm taking a step backwards here. You can probably tell why I've avoided javascript/ajax in shop so far.

Could it be feasible to request & return multiple parts of the page?

{
    "SideCart": "<div>side cart content here </div>",
    "Cart": "<div>cart content here </div>",
}

...is that even a good idea? You could still achieve the same thing with separate ajax calls.

Should we be thinking about this in a more API-based approach? i.e. returning the data in a form like:

{
"cart" : {
 "Currency":"USD"
 "Items":[
 124: {"Title":"t-shirt","Quantity":"3","UnitPrice":"23.14"},
 125: {"Title":"shorts","Quantity":"1","UnitPrice":"45.50"}
]
}
}

I can see this would introduce more burden on the shop javascript library, as you'd need to handle updating html, possibly using a different set of js-based templates. I'm guessing this approach could be considered client-side 'view' when thinking about MVC.

@markguinn What is your gut direction to take?

markguinn commented 10 years ago

Thanks for the feedback, @jedateach! Sorry in advance for the long response.

RE: triggerEvent: That add method was just an example more or less based off the one in ShoppingCart_Controller. The triggerEvent method would trigger a clientside event, allowing more customisation of clientside stuff (Use case: I click add to cart on a product on a category page, but it has variations. I open a dialog to display the options and listen for the "addtocart" event to close the modal).

RE: Returning multiple regions: Absolutely. I intend this to work both directions so server code could call pushRegion to add one or more regions of HTML to the response (referenced by template includes). The client-side code could also "pull" a region (Use case: rather than the normal SideCart template if my site displayed shopping cart status in a different way, I might want to pull a different region with any calls to the shopping cart). I envisioned it being able to either return json as you indicated above or possibly a similar syntax in pure html. Check out the second page of my google doc (https://docs.google.com/document/d/1YGcDAZIBG2mH_kDX5Q5wa6aWl7KpIl_f9P8tflR_21U/edit?usp=sharing) for more details on what I'm thinking there. This approach allows developers to easily hook into existing functionality for very different themes and markup structures.

RE: Syntax: I get what you're saying. The chained syntax was just a possible evolution of an idea. That kind of "builder" pattern in used a lot in the Android SDK, and similar chaining is used a lot in Javascript (angularJS, etc) but you're right that it's not very familiar in Silverstripe. It's probably more important to come up with something intuitive for SS devs.

RE: Extension point: I think I misunderstood the server-side approach you and @wilr were suggesting. I thought you meant the opposite of what your'e suggesting - an extension to Controller (or a RequestFilter) that would further automate ajax responses. Basically, standardising and making generic the logic expressed in ShoppingCart_Controller::direct. I think what you're suggesting is a great idea though and I'll run with that architecture. I think some helper functions are also needed such as triggerEvent and pushRegion. If my additions start going farther than you want, we can just add the extension points as you suggest and push the helpers into a submodule. I do think most e-commerce sites will use ajax though, so I'd prefer to see them built into shop so that other submodules can standardise more in their approach to js.

RE: Server vs client views: My preference is to do templating on the server so that you only define templates once. If I were doing view construction on the client, I would be using something like AngularJS and SilverStripe's built-in REST support. My sense is we don't want to go that far or be that opinionated at this junction. However, if we built it extensively like we're talking about, one could pretty easily write an alternate ShoppingCart_AjaxController that would return JSON data right? That feels like cutting against the grain of the way the module is built though (in it's current state). I'm open to changing that stance though if others feel we should push more responsibility to the client-side now.

So, at long last, what about something more like this:

class ShoppingCart_Controller {
   function add($request){
      // do the stuff
      if ($request->isAjax()) {
         // the following three lines could probably also be condensed into a helper method?
         $response = $this->getAjaxResponse($request); // this would come from Controller extension
         $this->extend('onAddResponse', $response, $request, $success);
         return $response;
      } else {
         // normal logic would go here, in the case of the existing controller that's self::direct()
         // but could also be $this->renderWith or other calls
         return self::direct();
      }
   }
}

class ShoppingCart_Ajax {
   function onAddResponse($response, $request, $success) {
      // I would imagine getAjaxResponse to return a subclass of SS_HTTPResponse or
      // something like that so it would handle construction of the actual body itself
      $response->triggerEvent('addtocart', array(/* whatever */));
      $response->pushRegion('SideCart');
   }
}
markguinn commented 10 years ago

Here's my first crack at what this would look like on the server side if you're interested. We can still very much change this around. I just thought some code might help: https://github.com/markguinn/silverstripe-shop/commit/eb04dce3967cd8febb552b6befdf50e8ed07b341

markguinn commented 10 years ago

I've got a lot of this implemented here: https://github.com/markguinn/silverstripe-shop/commits/feature-ajax-framework And add to / remove from cart working with ajax on your bootstrap template: https://github.com/markguinn/silverstripe-bootstrap-shop/commits/patch-ajax And documented here: https://github.com/markguinn/silverstripe-shop/blob/feature-ajax-framework/docs/en/Ajax.md

I'd love some review/feedback on this direction before I go much further.

markguinn commented 10 years ago

I realised my method above isn't very extendable or flexible. I've modified it to look more like this (and more like your original suggestion), and to also use the injector to create AjaxHTTPResponse so that could be swapped out as well (e.g. to respond with xml) by simply subclassing and yml config.

    public function add($request) {
        if ($product = $this->buyableFromRequest()) {
            $quantity = (int) $request->getVar('quantity');
            if(!$quantity) $quantity = 1;
            $this->cart->add($product, $quantity, $request->getVars());
        }

        $this->extend('updateAddResponse', $request, $response, $product);
        return $response ? $response : self::direct();
    }

Where updateAddResponse in the decorator now handles everything.

markguinn commented 10 years ago

Any more thoughts on this @jedateach? If you don't like it, I can just add the extension points here and build the rest as a module.

markguinn commented 10 years ago

Looks like it didn't register automatically but this issue is affected by pull request #275. I'm copying the text from that issue here for reference.


This PR doesn't add full ajax support but it adds extension hooks to make that happen. In the mean time, so that these features can be used safely in modules, I've separated the actual features into a couple modules. I felt the ajax module itself could be useful outside of the shop module so I think we should probably leave it separate. If you want to merge the shop-ajax module into shop core to keep from having two sets of pull requests in the future, I'm totally fine with that.

jedateach commented 9 years ago

Thanks for your work on this Mark! Really appreciate the time/effort. Would you say we can now close this issue?

markguinn commented 9 years ago

If you're happy with this solution, I'm happy, Jeremy. I'm still open to merging the shop-ajax module into core though if you'd prefer. This gives users the most flexibility. I'll add shop-ajax as a suggested module.

jedateach commented 9 years ago

Can you clarify which approach you think gives the most flexibility? + why

For me, having just the hooks means that I don't need to worry about maintaining any javascript or related server side code.

markguinn commented 9 years ago

Sorry for being unclear. It is most flexible to have them in two different modules. This way if someone doesn't like the particular ajax approach I've chosen they can write their own ajax extensions. It sounds like we're on the same page here that leaving them separate is best.