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

Segmentation / Introduce CustomerSegmentationTemplate component #759

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

loic-d
Copy link
Contributor

@loic-d loic-d commented Feb 22, 2023

Background

Fixes https://github.com/Shopify/core-issues/issues/51784
We are building a new rendering UI extension for Customer Segmentation. We're introducing a new component called CustomerSegmentationTemplate that will be used to render segmentation templates in the Admin:

Screenshot 2023-02-22 at 3 09 10 PM

Solution

This PR adds the required interfaces for CustomerSegmentationTemplate in @shopify/ui-extensions/admin and @shopify/ui-extensions-react/admin. See code examples in PR.

🎩

🍏 CI. You can also generate a build (yarn build) and consume these new changes in your app extension.

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@github-actions

This comment has been minimized.

Copy link
Member

@henrytao-me henrytao-me left a comment

Choose a reason for hiding this comment

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

Code looks good 👍


export interface ExtensionPoints {
Playground: RenderExtension<StandardApi<'Playground'>, AnyComponent>;
'Admin::Customers::SegmentationTemplatesFirstTimeBuyers::Show': RenderExtension<
Copy link
Member

Choose a reason for hiding this comment

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

Why did we choose to do separate extension points for each of these, rather than the context being passed in as part of the extension API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lemonmade we just haven't thought about it. currently, each extension point maps to a category of templates.

Screenshot 2023-02-23 at 4 30 54 PM

Our extension could accept a templateCategory parameter as part of the API and return the right templates accordingly. This would also solve the problem of the "all" category. We'd have to run the extension for each category (instead of running extensions for each extension point).

How does that sound? Would you also use a union type instead of an enum to represent the categories?

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that the extension has to handle all categories? What happens if they don't? We'd just get a blank extension with no way to tell that it's blank no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We (Segmentation app) have to handle all categories. Admin will loop over a list of categories and render the extension with the current category as api. If our extension doesn't handle a category, the page will be blank. We'll make sure to handle all categories. This is maybe where a non-rendering extension could have been matter because we could have returned a map of templates where the key is the category.

For other 1P (Shopify Email, etc.) and 3P apps, their templates will be grouped under an "Apps" entry point. We will provide apps as category but they can basically ignore it.

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 it would be very messy to go down the route of passing them a category they might ignore. It would result in the wrong template being shown for the category or blank content. It actually seems a lot better to separate these to different extension points they can specifically opt in to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The category is really just for us. Can we have different private and public APIs?
On the Admin side we were thinking of populating categories just for our app (app ID or handle lookup).
Any other extension should go to the Apps category and the provided templateCategory doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed on Slack, TLDR: we don't know yet how/if categories will be used by 3P. It makes sense to keep things simple for now with a single extension point and the templateCategory passed in as a starting point. We can evolve this later


export interface ExtensionPoints {
Playground: RenderExtension<StandardApi<'Playground'>, AnyComponent>;
'Admin::Customers::SegmentationTemplatesFirstTimeBuyers::Show': RenderExtension<
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to wait until the conventions for extension point names is sorted out? Show is a term that they are using in that proposal currently, but it is different from checkout and I am not sure we have alignment on that part of the naming proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looped in @MeredithCastile. Like you suggested, we might even be able to merge all our extension points.

I'm thinking it could be admin.customers.segmentation-templates.show. This could be used by both our Admin Templates app, and 1P + 3P apps 🤔

import type {StandardApi} from '../standard/standard';
import type {ExtensionPoint as AnyExtensionPoint} from '../../extension-points';

export enum CustomerSegmentationFeature {
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 there needs to be some documentation about what these are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add more context. These are internal feature flags for our own Segmentation templates. Other 3P apps shouldn't rely on them and only use QL public features.

Once we open this to 3Ps, can we expose a new API interface without this property?

ExtensionPoint extends AnyExtensionPoint,
> extends StandardApi<ExtensionPoint> {
/** List of enabled segmentation query language features. */
enabledFeatures: [CustomerSegmentationFeature];
Copy link
Member

Choose a reason for hiding this comment

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

This is a guaranteed, single-item array of enabledFeatures — is this what you are after? I assume you at least want this to be CustomerSegmentationFeature[] instead.

However, is there no chance this list will ever change at runtime? In checkout, we already wrap all "read" APIs like this in a "subscribable" object that gives us the surface area to let them change over time (and to give partners a consistent API for subscribing to changeable data).

Copy link
Contributor Author

@loic-d loic-d Feb 23, 2023

Choose a reason for hiding this comment

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

As you can see, my code is rusty 😄

CustomerSegmentationFeature[] is what we need.

All these features map to a beta flag in Core. I guess the list of features could change at run-time if we trigger a refetch of the component's GQL query (wrapping the extension) and pull the updated beta flag values.

} from '@shopify/ui-extensions-react/admin';

function App({i18n, enabledFeatures, category}) {
if (category !== 're_engage_customers') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1,3 +1,4 @@
export type {I18n, I18nTranslate} from '../../shared';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're now re-exporting these types from the shared utility

@loic-d
Copy link
Contributor Author

loic-d commented Feb 23, 2023

@vividviolet @lemonmade this is ready for a second review:

  • move I18n to a shared file for both checkout and admin surfaces
  • use a single extension point (admin.customers.segmentation-templates.render) and provide the template category as part of the api. Meredith is on board 👍
  • use union type instead of enums
  • improve examples and inline comments

the outstanding question is for enabledFeatures. these are really meant to be temporary (weeks) while we rollout new features. they shouldn't be used by 1P or 3P apps other than our own Segmentation app. once we address some technical limitations on our customer data platform, we shouldn't even need them anymore. are you ok with the approach I took in this PR?

/**
* This defines the i18n.translate() signature.
*/
export interface I18nTranslate {
Copy link
Member

Choose a reason for hiding this comment

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

@kumar303 @marvinhagemeister do you know if any of the docs extraction stuff is depending on the exact file structure of our packages? This is on the shared UI extension package, so it won't impact checkout yet, but we will need to make sure it works as part of versioning.

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.

👍

@loic-d loic-d force-pushed the ld/customer-segmentation-template branch from 55c3cc0 to 2174598 Compare February 24, 2023 18:39
…nsions-react

Adds changeset

Updates comment

Fixes typo

Moves CustomerSegmentationTemplateComponent to extension-points file

Moves I18n to shared utility, Renamed extension points

Properly re-export I18n and I18nTranslate

Fixes examples, Renames prop to icon

Uses a type instead of an enum. Adds comments on enabled features

Drops Polaris from comment

Uses single extension point

Returns null

Better examples

Fixes conditional in example

Fixes QL string in examples

Uses render instead of show

Marks enabledFeatures as private and add __ prefix

Moves I18n to API

Uses camelCase for categories, Make categories private

Uses camelCase for enabled features type
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