canjs / place-my-order

PlaceMyOrder.com example with just CanJS
http://canjs.com
MIT License
0 stars 3 forks source link

Orders should not come back as a map of lists, only a list of all items. #11

Open justinbmeyer opened 9 years ago

justinbmeyer commented 9 years ago

it seems like the data is coming back as {new: [...], preparing: [..]}. We should not have a restful service return data this like way.

Instead, the view-model should be doing this grouping.

Also, if there are no preparing / new, etc, the <pmo-order-history-item> would not be rendered, and probably not show "no new orders".

    {{#if orders.value.new}}
      <pmo-order-history-item
        status="new"
        title="New Orders"
        orders="{orders.value.new}"
        empty-message="No new orders">
      </pmo-order-history-item>
    {{/if}}

    {{#if orders.value.preparing}}
      <pmo-order-history-item
        status="preparing"
        title="Preparing"
        orders="{orders.value.preparing}"
        empty-message="No orders preparing">
      </pmo-order-history-item>
    {{/if}}
daffl commented 9 years ago

It looks like this is being converted in the model Order.List at https://github.com/canjs/place-my-order/blob/master/app/models/order.js#L43.

chasenlehara commented 9 years ago

The “service” is just a list of orders, so the data isn’t grouped like that until you’re in the model.

We could move that logic to the viewmodel, I think we both thought it belonged in the model.

As for it not rendering, it does because those properties exist, even if they don’t have a length. You can see in the example app, where the “in delivery” section has no orders.

m-mujica commented 9 years ago

So, on a second thought those ifs are not needed at all; about the naming, I'll rename the component to pmo-orders-list, I think it makes more sense.

justinbmeyer commented 9 years ago

@chasenlehara it can be done in the model, but then you should probably have an Order.findAllGrouped or something. Model's findAll / findOne / save / destroy are an interface where you could ideally make a grid, and pass it a Model, and that grid could use those methods to work. This original findAll changes the meaning of findAll to not return a flat list. I don't want to encourage that.

For now, I would do it in the view-model. Thanks.

justinbmeyer commented 9 years ago

17 doesn't seem to close this all the way. I didn't see a chance to the view-model, unless I'm missing something.