-
Notifications
You must be signed in to change notification settings - Fork 27
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
Custom Views context hook adjustment #3292
Conversation
🦋 Changeset detectedLatest commit: 36dae62 The changes in this PR will be included in the next version bump. This PR includes changesets to release 37 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -45,6 +49,10 @@ export const renderCustomView = ( | |||
locale: props.locale, | |||
project: { | |||
key: props.projectKey, | |||
allAppliedPermissions: props.projectAllAppliedPermissions || [], |
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.
Writing the docs I found out we allow for custom permissions in Custom Application testing so I'm adding that possibility for Custom Views as well.
Deploy preview for application-kit-custom-views ready! ✅ Preview Built with commit 36dae62. |
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit 36dae62. |
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.
Nice, thanks for following up on this! 🤗
PS: changeset
allAppliedPermissions: props.projectAllAppliedPermissions || [], | ||
}, | ||
environment: { | ||
entryPointUriPath: CUSTOM_VIEW_HOST_ENTRY_POINT_URI_PATH, |
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.
💯
import { | ||
type TApplicationContext, | ||
useApplicationContext, | ||
} from '../application-context'; | ||
|
||
export type TCustomViewContext = { | ||
hostUrl: string; | ||
config: CustomViewData; |
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.
Nit: now that we have the data together, should we rename this to customViewConfig
?
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, that makes sense. Thanks 👍
Updated here: fa5fa67
…tom apollo client
@@ -44,6 +45,7 @@ type THostEventData = { | |||
}; | |||
|
|||
type TCustomViewShellProps = { | |||
apolloClient?: ApolloClient<NormalizedCacheObject>; |
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.
Also allow consumers to provide their custom Apollo client (same as with Custom Applications)
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.
💯 💯
Thanks
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.
Thanks for the enhancements 🙌
Just a reminder about the changeset 🤗
…ews resource accesses
@@ -118,6 +122,15 @@ function entryPointUriPathToResourceAccesses< | |||
}; | |||
} | |||
|
|||
function resolveCustomViewResourceAccesses<PermissionGroupName extends string>( |
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.
When writing the docs for Custom Views I noticed if a developer needs to calculate the permissions he would need to use something like this:
export const PERMISSIONS = entryPointUriPathToPermissionKeys(CUSTOM_VIEW_HOST_ENTRY_POINT_URI_PATH);
It seems confusing to me as the concept of entry point
does not exist for Custom Views, so I decided to propose the inclusion of this new helper target for Custom Views where they can get both the basic resource accesses and also the one for the additional groups.
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.
@emmenko please let me knot what you think about this 🙏
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.
Makes sense, thanks for thinking about that!
In the example you refer to "permission keys" but here the helper is about "resource accesses".
Maybe we can provide 2 helper functions for both:
computeCustomViewResourceAccesses
computeCustomViewPermissionKeys
Nit: compute
sounds a bit more appropriate than resolve
.
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.
Thanks for the suggestion.
I updated this part here: 96cc64e
environment?: Partial<TProviderProps<{}>['environment']>; | ||
user?: Partial<TProviderProps<{}>['user']>; |
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'm also adding the environment
and the user
as they also seem quite helpful when testing Custom Views with different permissions.
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.
Sorry for the late review 🙃
@@ -118,6 +122,15 @@ function entryPointUriPathToResourceAccesses< | |||
}; | |||
} | |||
|
|||
function resolveCustomViewResourceAccesses<PermissionGroupName extends string>( |
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.
Makes sense, thanks for thinking about that!
In the example you refer to "permission keys" but here the helper is about "resource accesses".
Maybe we can provide 2 helper functions for both:
computeCustomViewResourceAccesses
computeCustomViewPermissionKeys
Nit: compute
sounds a bit more appropriate than resolve
.
Co-authored-by: Nicola Molinari <nicola.molinari@commercetools.com>
Summary
Custom Views context hook adjustment
Description
In this PR I propose refactoring the
useCustomView
hook to not only return the Custom Views context but also the internal Application context. This way, Custom Views developers requirement some data from any of the context can access to them from the same hook (dedicated to Custom Views) without them needing to know we manage two context internally.