Closed MatteoPiovanelli-Laser closed 7 years ago
Should I review now, or should I wait for the work to be complete?
If you have time, I'd appreciate it if you could give it a quick look. We are making some changes for the sake of clarity, but maybe you will spot something we oversaw.
@HermesSbicego-Laser and I updated things a bit. We simplified the UI, moving away from using HTML5 css3 modals everywhere, because that could have easily become a nightmare when trying to alternate from a theme. We also fixed a few bugs here and there that we spotted.
Wow, that's a pretty big feature. Thanks for the contribution! I had lots of comments (because that's a lot of code).
I'm also still confused about the use of the word "element": we have something else in Orchard that is also called, confusingly enough, an element, and it seems like the shopping carts uses "item" for something similar.
Thanks for taking the time to review the code.
I made most changes you recommended (update to the PR will be out in a few minutes). Where I replied "ok" to your comments it was so that I would see that I had worked on that point.
A few things are still not finalized:
WishListController.AddToCart
: it uses model binding now, and verifies things about the wish list before adding the product to the cart. IWishlistServices
will change to reflect not always needing the user as a parameter.ShoppingCartController
. It may be worth to opne a seperate issue to see if it's possible to convert that so it does no use the form, but I feel that since we cannot know what will be in the extensions, we may be out of luck here. A rewrite may be possible such that extensions have a common portion that allows identificaition, and then they process the form themselves if needed, so that we may remove it form the controller. It may be possible to bind a dictionary similarly to what I did for WIshListController.AddToCart
.WishListListPart.Ids
: it's basically the same as ContentPickerField.Ids
, but in my own record rather than StringFieldIndexRecord
.Il the latest PR there are more fixes and features:
IWishListsUIServices
.WishListListPart
(and WishListListPartRecord
): since in WishListItemPartRecord
I have the id of the "container" wish list anyway, the list of items in a wish list is taken by querying on that. I think this also makes things cleaner, because this way when the items in a wish list are updated (add/remove) there is only one place that stores the info, rather than two that need to be kept in synch.WishListsAuthorizationEventHandler
. The default behaviour is "wish lists are all private at all times". See IWishListsPermissionsService
and its provided implementation.I updated the PR.
The views returned by the Index
method. This way it's easy to use placement info to only show what we want. E.g. something like this would result in in showing the detail only when "asking" for a specific wish list, and only the list of wish lists on the "generic" path to the action:
<Match Path="/wishlists">
<Place Parts_ListOfWishLists="Content:1"/>
<Place Parts_WishListList="-"/>
<Place Parts_WishListsActions="-"/>
</Match>
<Match Path="/wishlists/*">
<Place Parts_ListOfWishLists="-"/>
<Place Parts_WishListList="Content:1"/>
<Place Parts_WishListsActions="-"/>
</Match>
I am handling the case where a user has no wish list they are allowed to see with a 404.
public wish lists: since this is a pretty big feature already, I'd rather leave those considerations for separate/additional features later on, especially since those can be become as complex as we wish.
Thanks. There are still conflicts to solve however.
I fixed the merge conflicts, and also used the new cart events in the WishListsController.AddToCart
method
Thanks!
thanks for reviewing this and merging it in,
117 PR on wish lists.
This is partially a WIP. There are still some UI things we are working on, especially regarding the "add to wishlist" menu from products' display views. We are also looking into notusing javascript for the view of the "add to wishlist" shapes.
The following functionality is in place for authenticated users. Unauthenticated users get redirected to the logon page.
/wishlists
route: