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.

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

Add the purchase option extensions template #142

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

Conversation

JulienHe
Copy link

@JulienHe JulienHe commented Oct 1, 2024

Background

The selling strategies is introducing 2 new admin action:

  • admin.product-purchase-option.action.render
  • admin.product-variant-purchase-option.action.render

We are shipping a new template so apps partners can use it and have a first starter when they want to work with these new extensions

Solution

We are adding the new extensions to the available templates.

Checklist

  • I have 🎩'd these changes
  • I have squashed my commits into chunks of work with meaningful commit messages

@JulienHe JulienHe requested a review from a team as a code owner October 1, 2024 14:00
Copy link
Member

@vividviolet vividviolet left a comment

Choose a reason for hiding this comment

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

We actually shouldn't merge this until the targets are in GA

@JulienHe
Copy link
Author

JulienHe commented Oct 2, 2024

We actually shouldn't merge this until the targets are in GA

Yes, we have an epic with what need to be shipped first.

@JulienHe JulienHe changed the title Add the purchase option extensions Add the purchase option extensions template Oct 3, 2024
@JulienHe JulienHe self-assigned this Oct 3, 2024
"dependencies": {
"react": "^18.0.0",
"@shopify/ui-extensions": "unstable",
"@shopify/ui-extensions-react": "unstable"
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be pointing to the latest stable calver (10-24) not to unstable

Copy link
Author

Choose a reason for hiding this comment

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

Thanks~ Will change.

Comment on lines 165 to 171
extend(PRODUCT_TARGET, (root, {i18n, close, data}) => {
PurchaseOptionsAction(root, {i18n, close, data});
});

extend(PRODUCT_VARIANT_TARGET, (root, {i18n, close, data}) => {
PurchaseOptionsAction(root, {i18n, close, data});
});
Copy link
Contributor

@MitchLillie MitchLillie Oct 3, 2024

Choose a reason for hiding this comment

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

These should be extension instead of extend (many of the other templates are wrong and will be updated soon)

Also the export default and separate file comment applies here too

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for giving all these insight, I changed the templates :)

Comment on lines 22 to 28
reactExtension(PRODUCT_TARGET, () => (
<AdminSubsAction extension={PRODUCT_TARGET} />
));

reactExtension(PRODUCT_VARIANT_TARGET, () => (
<AdminSubsAction extension={PRODUCT_VARIANT_TARGET} />
));
Copy link
Contributor

@MitchLillie MitchLillie Oct 3, 2024

Choose a reason for hiding this comment

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

I am checking on this but I think We need two separate files, each with an export default reactExtension.... They can both import and share a third file with the actual component if you like.

Why -- because we have a future change to our build system that uses the default export from the source files to create named exports in the build files.

Copy link
Author

Choose a reason for hiding this comment

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

Changed the templates, thanks for catching it!

Copy link
Contributor

@MitchLillie MitchLillie left a comment

Choose a reason for hiding this comment

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

LGTM!

"license": "UNLICENSED",
"dependencies": {
"react": "^18.0.0",
"@shopify/ui-extensions": "2024.10.x",
Copy link
Member

Choose a reason for hiding this comment

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

I think this has to be set to unstable as the new targets weren't part of the 2024-10 release

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, good catch

import {reactExtension} from '@shopify/ui-extensions-react/admin';
import PurchaseOptionsActionExtension from './PurchaseOptionsActionExtension';

reactExtension('admin.product-purchase-option.action.render', () => (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
reactExtension('admin.product-purchase-option.action.render', () => (
export default reactExtension('admin.product-purchase-option.action.render', () => (

import {extension} from '@shopify/ui-extensions/admin';
import PurchaseOptionsActionExtension from './PurchaseOptionsActionExtension';

extension(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extension(
export default extension(

import {reactExtension} from '@shopify/ui-extensions-react/admin';
import PurchaseOptionsActionExtension from './PurchaseOptionsActionExtension';

reactExtension('admin.product-variant-purchase-option.action.render', () => (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
reactExtension('admin.product-variant-purchase-option.action.render', () => (
export default reactExtension('admin.product-variant-purchase-option.action.render', () => (

import {extension} from '@shopify/ui-extensions/admin';
import PurchaseOptionsActionExtension from './PurchaseOptionsActionExtension';

extension(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extension(
export default extension(

Copy link

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

Just a nit / question, but otherwise LGTM

};

function handleSave() {
// This is where you can use the sellingPlanGroupsCreate and sellingPlanGroupsUpdate mutations

Choose a reason for hiding this comment

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

Will running this for products or variants change how a dev would call these mutations? If so, should we have an example of how to tell which one we're rendering?

Copy link
Member

Choose a reason for hiding this comment

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

I think the sellingPlanGroupsUpdate mutation doesn't need to know about product vs variant. You make a good point that this example isn't enough though. We'd need to actually call the mutations for adding variants and products correctly and those are different. Could we add a complete example @JulienHe ?

I wrote some pseudo code here where the extension passes in the right mutation to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants