Skip to content
This repository was archived by the owner on Apr 10, 2025. It is now read-only.

wishlist concept#18

Open
aseyfarth wants to merge 2 commits intomainfrom
feature/wishlist
Open

wishlist concept#18
aseyfarth wants to merge 2 commits intomainfrom
feature/wishlist

Conversation

@aseyfarth
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@SGD1953 SGD1953 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server side part or it looks a bit too complex to me.

next();
});

server.get('Event', (req, res, next) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be post

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

htmx requires a solid html form which created some trouble with the layout. Other ways would require interfering with the htmx request execution. All the solutions I found were way too complicated for what I want to achieve it, compared to using a GET, although a POST would be logical in regards to the endpoint purpose

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, something we need to look into. In my view we should only jeopardise a clean architecture for very good reasons.

*/
function post(req, res, next) {
if (req.httpMethod === 'POST') {
if (request.httpMethod === 'POST') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this here on purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, without it POST requests are not working at all. Initially I tried the event as POST.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry my fault. I degutted the SFRA server and rely more on dw standard objects. maybe we can pass in the actual request.

model = request.custom.model; // eslint-disable-line no-undef

return `
${hookMgr.callHook('wishlist.template', 'productIcon', model.product.ID)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I need a bit more explanation, but I'm not sure I'm in love with this hook-pattern. What do we need it for?

Copy link
Collaborator Author

@aseyfarth aseyfarth Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intended as idea to have the plugin separated from the storefront cartridge.
In this file here, I just reused it, as it was created already. But looking at the tile partial I wanted to avoid having something like

const heartIcon = require('..../heartIcon');
heartIcon.createModel(....);
heartIcon.template(....);

as this would mess up the storefront with code from my "optional" extension

Copy link
Owner

@hnestmann hnestmann Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like hooks to be able to place a generic "spot" on a template which a plugin can fill.

Ideally though, we can come up with a way where you "click not code" and drag the individual wishlist controls on the page using page designer

But how would one do this for tiles?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move tiles to OD as well, but currently this would make it super-slow due to the uncached first include. In the meanwhile, the tile could have an array plugins which would accept partials (i.e. via a registerPlugin() method) which would then be rendered at a defined spot. Or alternatively, you could simply extend the tile.

@@ -0,0 +1,15 @@
<isdecorate template="decorator/main">
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ISML was just as example placeholder to see the items on the wishlist.

@@ -0,0 +1 @@
<isinclude url="${URLUtils.url('Tile-Show', 'pid', item.productID)}"/> No newline at end of file
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ISML was just as example placeholder to see the items on the wishlist.
ISML free approach can be taken later.

@@ -0,0 +1,42 @@
<a class="wishlist-header-icon ${pdict.filledWishlistClass}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can port that markup into script.


const ProductList = require('dw/customer/ProductList');

server.get('HeaderIcon', (req, res, next) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, I am just wondering if those header includes do really all need to live in their own include or if we could just have one (which will have Account, Wishlist, Cart etc.)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For account I use PD to have a "cached" version for guests. Other then that I agree, although it's less of an issue for YSH as we ideally render the header just once per session (if all links are htmxed correctly)

@SGD1953
Copy link
Collaborator

SGD1953 commented Feb 27, 2025

@aseyfarth what's the status here, can we get this into a mergeable shape?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants