Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wishlist template ts migration #120

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Wishlist template ts migration #120

wants to merge 21 commits into from

Conversation

DevAOC
Copy link
Collaborator

@DevAOC DevAOC commented Nov 8, 2024

No description provided.

@DevAOC DevAOC requested a review from rdraward November 8, 2024 20:50
rdraward
rdraward reviewed Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@rdraward rdraward left a comment

Choose a reason for hiding this comment

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

Permissions for calling Gadget from the extension are not correct
It could also be nice to have some error handling in the theme extension (but that can be done separately)

shopify/wishlist-template/api/models/shopifyShop/actions/install.ts Show resolved Hide resolved
@DevAOC DevAOC requested a review from rdraward November 18, 2024 18:07
rdraward
rdraward reviewed Nov 22, 2024
View reviewed changes
Copy link
Collaborator

@rdraward rdraward left a comment

Choose a reason for hiding this comment

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

Clicking on the icon or text in the theme extension wishlist does not work:

CleanShot.2024-11-22.at.09.37.30.mp4

shopify/wishlist-template/web/routes/index.tsx Outdated Show resolved Hide resolved
shopify/wishlist-template/web/routes/index.tsx Show resolved Hide resolved
shopify/wishlist-template/extensions/wishlists/src/components/NewWishlist.tsx Outdated Show resolved Hide resolved
shopify/wishlist-template/extensions/wishlists/src/components/NewWishlist.tsx Show resolved Hide resolved
shopify/wishlist-template/api/actions/sendInStockEmails.ts Outdated Show resolved Hide resolved
shopify/wishlist-template/api/actions/startCustomerUpdates.ts Outdated Show resolved Hide resolved
shopify/wishlist-template/extensions/wishlists/src/api.ts Outdated Show resolved Hide resolved
@DevAOC DevAOC requested a review from rdraward November 26, 2024 15:19
rdraward
rdraward reviewed Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@rdraward rdraward left a comment

Choose a reason for hiding this comment

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

Works pretty well now! No major issues when going through 🎩

One note: after noodling around in the customer accounts area and looking at Shopify's example (https://shopify.dev/docs/apps/build/customer-accounts/full-page-extensions/build-new-pages?extension=react) I think we need another extension that links to the wishlist page in the customer accounts portal? I don't think there is a way for me to get there without the CLI link right now?

Some other notes:

  • we can remove the step in the template setup instructions about adding a new toml file
  • buttons in storefront still don't work when clicking on icon or text

shopify/wishlist-template/api/actions/sendWishlistEmail.ts Outdated Show resolved Hide resolved
shopify/wishlist-template/api/models/shopifyCustomer/actions/create.ts Show resolved Hide resolved
@DevAOC
Copy link
Collaborator Author

DevAOC commented Nov 28, 2024

I wasn't able to fix the issues with the text and the icon. Would you be able to help me look into it?

@rdraward
Copy link
Collaborator

I wasn't able to fix the issues with the text and the icon. Would you be able to help me look into it?

A console error is thrown when I click on the text or button. In handleWishlistClick we should use e.currentTarget instead of e.target. currentTarget gives us the element that the event listener is bound to, target is the clicked element (we get the child element first because of event bubbling)

@DevAOC
Copy link
Collaborator Author

DevAOC commented Nov 28, 2024

I wasn't able to fix the issues with the text and the icon. Would you be able to help me look into it?

A console error is thrown when I click on the text or button. In handleWishlistClick we should use e.currentTarget instead of e.target. currentTarget gives us the element that the event listener is bound to, target is the clicked element (we get the child element first because of event bubbling)

I didn't know there was such thing as currentTarget. You learn something new every day. You would think that it would be documented and recommended by ChatGPT

@DevAOC DevAOC requested a review from rdraward November 28, 2024 19:12
rdraward
rdraward approved these changes Dec 2, 2024
View reviewed changes
Copy link
Collaborator

@rdraward rdraward left a comment

Choose a reason for hiding this comment

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

🎩 ✅
I think we probably want to add another mini extension to allow for wishlist navigation eventually, but don't worry about that now

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

@rdraward rdraward rdraward approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DevAOC @rdraward