-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat: add discount details block #1920
base: unstable
Are you sure you want to change the base?
Conversation
9db8d4b
to
f3ef83c
Compare
f3ef83c
to
4c4223b
Compare
4dc1ca3
to
580536f
Compare
We detected some changes in |
f508625
to
751d123
Compare
packages/ui-extensions/docs/surfaces/admin/staticPages/targets-overview.doc.ts
Outdated
Show resolved
Hide resolved
cf6e100
to
d953488
Compare
2c5e27a
to
86fea47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Hey folks, I worked on the UI extension target naming conventions, and on the extension target for partners to build cart validation function setting UIs, admin.settings.validation.render
.
My assumption was that future UI extensions collecting settings (metafields) for functions would follow the same pattern. IIUC, the proposed extension target is the same basic concept as the cart validation settings UI extension, so I think it fits the bill. To match how we named cart validation, I would have expected something like admin.discount-details.render
— you are providing content to render for a discount, which unlike cart validations, is not a nested concept (hence why it has a simpler 3-segment name than the the 4-segment one that is used for most other admin targets).
Can you talk through why you feel .function-metafields
is required here? From my perspective, the developer is really providing the whole page here, and the fact that the UI they provide is nested in a card is just a current, changeable design decision (e.g., these could in the future become multiple cards, the app icon might move to other places, etc). Also, I'd love to read any documentation you have on how this extension target compares to admin.settings.validation.render
in other ways — in particular, how the extension will be informed of its initial data, and how it will write that data back to Shopify/ how it will interact with the admin’s contextual save bar flow.
package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this was created and committed accidentally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, apologies. Will remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has nothing to do with the target name, but I am really surprised to see that we don't put the app's branding in a significantly more prominent place on this page; I would have expected the page heading would contain the app icon, since the app is the one that is effectively creating the entire resource. Shopify is providing a lot of content to the page, but the developer provides the function and UI extension that make this discount a thing, and we will presumably annotate the discount with its app icon in other merchant-visible locations, like the order timeline, or in future function performance dashboards we provide to merchants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point. I wonder if the reasoning was because a developer can always create an app if they want more control over the branding / ui. I do believe it was deliberated on for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally there is an ongoing effort to standardize and improve app attribution in the admin. This effort is being led by @RileyMacIntosh. As soon as a pattern is established, our team plans on adopting it.
We opted for
This extension gets it's data from the DiscountNode query which fetches a price rule. Said price rule can contain a metafield, which is passed to the extension and the developer can then allow the merchant to configure this metafield. We leverage the FunctionSettings component to allow the developer to pass the data entered by the merchant back to us (Web), before Web fires the mutation to create the price rule. There's a bit more info on the reasoning behind the target name here: https://docs.google.com/document/d/1r9mRFJi-cNz2GlbETPX53EMvlBgAMLLqbozws9DSBhw/edit#bookmark=id.kbfcs1ypqgus And our tutorial that goes along with the feature might shed light on the purpose of the extension: https://shopify-dev.shopify-dev-z8uc.shahriar-rahman.us.spin.dev/docs/apps/build/discounts/build-ui-extension?extension=javascript Let me know if you have any questions or if something doesn't make sense. Happy to chat 😄 |
Thanks for the context, @devisscher. I left a comment in a Slack thread where we were discussing target naming, but my TL;DR is that I think we should not introduce an additional segment here that we did not introduce for the validation UI extension target.
Can you describe specifically how the developer receives the data? For example, in the Validation UI extension, the developer receives a Is all of the above also true for this target, but with |
@lemonmade it is the same pattern as for validation but for discount, that's correct. We add the data to the extension here and it contains the functionId, the priceRuleId and the metafields. |
Background
Adding the UI extension block for discount details.
The extension target is named:
admin.discount-details.function-settings.render
Target is behind an app beta flag,
extension_only_discounts_app
Fixes: https://github.com/Shopify/core-issues/issues/71151, https://github.com/Shopify/core-issues/issues/74963
Tech design for this project: https://docs.google.com/document/d/1r9mRFJi-cNz2GlbETPX53EMvlBgAMLLqbozws9DSBhw/edit#bookmark=id.kbfcs1ypqgus
Solution
Added based on the instructions found here: https://github.com/Shopify/app-ui/wiki/Creating-new-admin-extensions#introducing-new-extension-targets-in-admin
🎩
Checklist